Adds longas, maximas and non-standard tweaks to MultiMeasureRest

Adds longas, maximas and non-standard tweaks to MultiMeasureRest

[Carl, 2011-05-18 13:52:19]

This looks generally good to me.

I'm concerned about the name "duration-log-list".  I've commented more on it
below.

Thanks,

Carl

http://codereview.appspot.com/4536068/diff/1/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/4536068/diff/1/scm/define-grob-properties.scm#n...
scm/define-grob-properties.scm:232: (duration-log-list ,list? "List of
@code{duration-log}.")
This name is nice and generic, which is good.  Bit it has no information content
as far as I can see.  Can we make it more explicit by changing either the name
(to something like usable-duration-logs) or the description (to something like
"List of duration-logs that can be used in typesetting the grob")?

As I read through things I couldn't understand what duration-log-list was for
until I read the code (and implied it from the regression test).

[Bertrand Bordage, 2011-05-18 13:54:43]

Just one regtest change :
in multi-measure-rest.ly, the two longas of the third measure are tranformed
into a maxima (as expected).

Regards,
Bertrand

[Bertrand Bordage, 2011-05-18 16:10:12]

> This name is nice and generic, which is good.  Bit it has no information
content
> as far as I can see.  Can we make it more explicit by changing either the name
> (to something like usable-duration-logs) or the description (to something like
> "List of duration-logs that can be used in typesetting the grob")?

I agree. I like "usable-duration-logs". It will be included in the next patch
set.

[benko.pal, 2011-05-18 19:48:30]

http://codereview.appspot.com/4536068/diff/1/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/4536068/diff/1/scm/define-grobs.scm#newcode1305
scm/define-grobs.scm:1305: (duration-log-list . (0 -1 -2 -3))
On 2011/05/18 13:54:43, Bertrand Bordage wrote:
> Just one regtest change :
> in multi-measure-rest.ly, the two longas of the third measure are tranformed
> into a maxima (as expected).

I couldn't apply the patch to a recent tree, so I'm just guessing,
and am a bit nervous, as LilyPond's maxima rest is a misunderstanding.
do you mean a fat dash across three spaces?
a true maxima rest consists of two longa rests.
the best would perhaps be to leave it out completely:
(duration-log-list . (0 -1 -2))

[Bertrand Bordage, 2011-05-18 20:05:12]

I applied it to the more recent git HEAD without any problem.

By maxima, I mean the glyph "rests.M3".
I chose to use maximas because they are easier to read than two longas with many
space in between.
In such cases, this is mandatory :
{ \compressFullBarRests \time 4/1 R\longa*10 }

[Bertrand Bordage, 2011-05-18 20:08:00]

Sorry, I misread your first sentence. Forget the first line of my previous post.

[benko.pal, 2011-05-19 15:43:06]

> By maxima, I mean the glyph "rests.M3".
> I chose to use maximas because they are easier to read than two longas
> with many space in between.
> In such cases, this is mandatory :
> { \compressFullBarRests \time 4/1 R\longa*10 }
>
> http://codereview.appspot.com/4536068/
>

ok, I fully agree.  sorry for the noise.

p

[Bertrand Bordage, 2011-05-26 21:04:30]

Nitpicks done :
"duration-log-list" changed for "usable-duration-logs" (Thanks Carl !)
Whitespaces and tabs removed.

Bertrand

[Carl, 2011-05-30 02:07:40]

The code looks fine in general, but I question two of the properties that have
been added for MultiMeasureRest.

http://codereview.appspot.com/4536068/diff/19001/lily/multi-measure-rest.cc
File lily/multi-measure-rest.cc (right):

http://codereview.appspot.com/4536068/diff/19001/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:329: "longest-church-rest "
I'm not sure I understand how longest-church-rest interacts with \
usable-duration-logs.

Why can't longest-church-rest just be the smallest value in
usable-duration-logs?  Why do we need a separate property for this?

Also, why do we need a grob property for measure-duration-log?  The length of a
measure is a context property of the Timing context; I don't see a reason to
have the possibility of having a different measure duration in the time
signature and in the multi-measure rest grob.

[Bertrand Bordage, 2011-05-30 10:56:01]

longest-church-rest is the longest rest displayed in church rests.
One may want to only print a whole rest in a breve measure but print breve and
longa rests in a church rest.
But I agree with you, it can be changed for the min value of
usable-duration-logs.

You're right for measure-duration-log. This will be changed in a close future.

[Bertrand Bordage, 2011-06-02 18:14:54]

Carl's suggestions done !

Bertrand

[Neil Puttock, 2011-06-02 18:44:21]

http://codereview.appspot.com/4536068/diff/10008/lily/multi-measure-rest-engr...
File lily/multi-measure-rest-engraver.cc (right):

http://codereview.appspot.com/4536068/diff/10008/lily/multi-measure-rest-engr...
lily/multi-measure-rest-engraver.cc:232: last_rest_->set_property
("measure-length", sml);
This duplicates the setting for the left bound (the command column at the start
of the bar).

(see next comment)

http://codereview.appspot.com/4536068/diff/10008/lily/multi-measure-rest.cc
File lily/multi-measure-rest.cc (right):

http://codereview.appspot.com/4536068/diff/10008/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:125: SCM sml = me->get_property ("measure-length");
SCM sml = dynamic_cast<Spanner *> (me)->get_bound (LEFT)->get_property
("measure-length")

http://codereview.appspot.com/4536068/diff/10008/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/4536068/diff/10008/scm/define-grobs.scm#newcode...
scm/define-grobs.scm:1307: (round-to-longer-rest . #f)
remove

[Bertrand Bordage, 2011-06-02 19:58:31]

Thanks, 'tis done.

[Neil Puttock, 2011-06-04 16:01:14]

LGTM.

http://codereview.appspot.com/4536068/diff/17008/input/regression/multi-measu...
File input/regression/multi-measure-rest-standard.ly (right):

http://codereview.appspot.com/4536068/diff/17008/input/regression/multi-measu...
input/regression/multi-measure-rest-standard.ly:20: }
newline

http://codereview.appspot.com/4536068/diff/17008/input/regression/multi-measu...
File input/regression/multi-measure-rest-tweaks.ly (right):

http://codereview.appspot.com/4536068/diff/17008/input/regression/multi-measu...
input/regression/multi-measure-rest-tweaks.ly:14: }
newline

http://codereview.appspot.com/4536068/diff/17008/lily/multi-measure-rest.cc
File lily/multi-measure-rest.cc (right):

http://codereview.appspot.com/4536068/diff/17008/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:131: int measure_duration_log = static_cast <int>
(ceil (duration_log));
while I'd be happy with static_cast (mainly since it's easier to see where
casting occurs) I think we have an (admittedly undocumented) coding style which
stipulates functional casting:

int (ceil (duration_log));

http://codereview.appspot.com/4536068/diff/17008/scm/define-grob-properties.scm
File scm/define-grob-properties.scm (right):

http://codereview.appspot.com/4536068/diff/17008/scm/define-grob-properties.s...
scm/define-grob-properties.scm:706: @code{usable-duration-logs}. For example,
displays a breve instead of a whole
@code{usable-duration-logs}.  For

[Bertrand Bordage, 2011-06-06 10:13:52]

Thanks again, Neil !
I applied these.

[Graham Percival (old account), 2011-06-08 19:09:03]

Please send me the final version with
  git format-patch origin
so that I can push it.

[Neil Puttock, 2011-06-12 14:41:40]

On 6 June 2011 11:13,  <bordage.bertrand@gmail.com> wrote:
> Thanks again, Neil !
> I applied these.

Thanks.

There's one more static_cast here:

+ length = static_cast <int> (pow (2, -i));

Cheers,
Neil

[Bertrand Bordage, 2011-06-13 20:02:44]

Yes, thanks. 'Tis done.
Bertrand

[Graham Percival (old account), 2011-06-14 19:10:30]

thanks, pushed.

[dak, 2011-07-26 13:08:09]

I would suggest reverting this patch as "needs work" for now.

http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc
File lily/multi-measure-rest.cc (right):

http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:125: SCM sml = dynamic_cast<Spanner *>
(me)->get_bound (LEFT)
There is no good way to get around the cast?

http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:130: double duration_log = -log2
(ml.Rational::to_double ());
log2 is a floating point operation not guaranteed to be exact.  One usually uses
a loop for figuring out a proper integer value.  It is also not clear to me that
this will pick out the right kind of rest always.

For example, what to do about 3/2?  We either get 2/1 rests, or 1/1 rests, and
it is not clear to me that this rounding choice is necessarily the same as that
for 3/4.

I think it might be saner to just have an overrideable lookup list for exact
meters (and 4/4 is not necessarily the same as 2/2 here), and revert to a
separately configurable default otherwise, likely a whole rest.

http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:140: int list_elt = scm_to_int (scm_list_ref
(duration_logs_list, scm_from_int (i)));
Iterating through a list as if it were an array is a no-no.  This makes the
operation O(n^2) instead of O(n) and obfuscates what happens.  Check other code
examples for how to iterate through a list.

http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:229: for (int i = 0; i < scm_to_int (scm_length
(duration_logs_list)); i++)
Again: don't loop through a list by indexing it like an array.

http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:246: length = int (pow (2, -i));
Don't use floating point arithmetic.  This is just 2<<-i unless i is positive. 
One should exit the loop _before_ that happens, otherwise length will be
undefined with the integer variant.  With the floating point variant, length
becomes 0 before exiting the loop with i==1.  I doubt that is intentional.

http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:254: Stencil r (musfont->find_by_name ("rests." +
to_string (k)));
This may calculate a name "rests.-1" instead of the valid "rests.M1".  Use
Rest::glyph_name instead, though it may also need fixing in that regard.

[Bertrand Bordage, 2011-07-26 13:20:45]

Okay, I will fix these before the end of the day.

Thanks for taking time to do the review !
Bertrand

[dak, 2011-07-26 16:30:43]

http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc
File lily/multi-measure-rest.cc (right):

http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:254: Stencil r (musfont->find_by_name ("rests." +
to_string (k)));
On 2011/07/26 13:08:11, dak wrote:
> This may calculate a name "rests.-1" instead of the valid "rests.M1".  Use
> Rest::glyph_name instead, though it may also need fixing in that regard.

This comment was erroneous: find_by_name actually converts - into M by itself. 
However, it might still be a good idea to use Rest::glyph_name in order to get a
glyph with the right style for the document.

[Bertrand Bordage, 2011-07-26 22:47:53]

>
http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#n...
> lily/multi-measure-rest.cc:130: double duration_log = -log2
> (ml.Rational::to_double ());
> log2 is a floating point operation not guaranteed to be exact.  One usually
uses
> a loop for figuring out a proper integer value.  It is also not clear to me
that
> this will pick out the right kind of rest always.
> 
> For example, what to do about 3/2?  We either get 2/1 rests, or 1/1 rests, and
> it is not clear to me that this rounding choice is necessarily the same as
that
> for 3/4.
> 
> I think it might be saner to just have an overrideable lookup list for exact
> meters (and 4/4 is not necessarily the same as 2/2 here), and revert to a
> separately configurable default otherwise, likely a whole rest.

This sounds complex... I agree with you, but I don't know if others will since
it adds a heavy autobeaming-like system.
I need another day (or couple of days) to implement this feature.

Anyway, I made all the other changes.
Feel free to critisize, I still need to learn much about C/Scheme/LilyPond.

Bertrand

[Keith, 2011-07-27 03:51:32]

On 2011/07/26 22:47:53, Bertrand Bordage wrote:
> >
> > For example, what to do about 3/2?  We either get 2/1 rests, or 1/1 rests,
and
> > it is not clear to me that this rounding choice is necessarily the same as
> that
> > for 3/4.
> > 
> > I think it might be saner to [...]

> This sounds complex... 

How it works now is a little complex, but reasonable.

Both 3/4 and 2/4 have measure_duration_log=1 ask for half/minim rests, rounding
to shorter, but these are not on the default usable-duration-logs, so we use the
closest usable which are whole/semibreve.  Correct.

3/2 has measure_duration_log=0 ask for whole rests by default.

If I want have my 3/2 music use breve multi-measure rests, I can
round-to-longer-rest=##t.  If I want my 4/2 music to use whole/semibreve rests
(more likely) I need to cut some entries from usable-duration-logs -- I hope the
Notation Reference teaches me how to do this.

P.S. Bummer about the missing log2() in freeBSD; they seem to have a tracked bug
about it.

[Keith, 2011-07-27 04:13:47]

http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc
File lily/multi-measure-rest.cc (right):

http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:131: int measure_duration_log = int (ceil
(duration_log));
To me, having done numerical work, the chain of functions you use, int( ceil(
-log2(x))), is easily recognizable as "find the smallest n so that 1 / 2^n <=
x".

While log2 is inexact, the results when x is a power of 2 are exact
(IEEE-compliant floating-point represents integers and integer powers of 2
exactly) so reliably 3/4 and 2/4 both come out 1 (half/minim rest).

http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:132: if (round && duration_log - measure_duration_log
< 0)
This, however, makes me pause, then wonder why you didn't use the same idiom
 if (to_boolean(me->get_property ("round-to-longer-rest"))
   measure_duration_log = int (floor (duration_log));
 else
   measure_duration_log = int (ceil (duration_log));

[Bertrand Bordage, 2011-07-27 16:52:28]

This is really dirty, but here's an update that does what you want (I hope).
If you have ideas to clean a bit this mess...

Thanks,
Bertrand

[dak, 2011-07-27 17:25:58]

http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc
File lily/multi-measure-rest.cc (right):

http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:131: int measure_duration_log = int (ceil
(duration_log));
On 2011/07/27 04:13:48, Keith wrote:
> To me, having done numerical work, the chain of functions you use, int( ceil(
> -log2(x))), is easily recognizable as "find the smallest n so that 1 / 2^n <=
> x".
> 
> While log2 is inexact, the results when x is a power of 2 are exact
> (IEEE-compliant floating-point represents integers and integer powers of 2
> exactly) so reliably 3/4 and 2/4 both come out 1 (half/minim rest).

log2 is a transcedental operation, it is not guaranteed to be exact.  logb would
be (it just returns the exponent from the floating point representation).

Anyway, it seems like log2 is not universally available, so we should avoid it. 
The same is likely true for logb.

http://codereview.appspot.com/4536068/diff/37002/lily/multi-measure-rest.cc
File lily/multi-measure-rest.cc (right):

http://codereview.appspot.com/4536068/diff/37002/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:241: length = (2 << -i) / 2;
The division by 2 changes the result.  Not that I understand too well what it is
supposed to be doing.

[MikeSol, 2011-07-27 17:49:01]

http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc
File lily/multi-measure-rest.cc (right):

http://codereview.appspot.com/4536068/diff/30001/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:131: int measure_duration_log = int (ceil
(duration_log));
On 2011/07/27 17:25:59, dak wrote:
> On 2011/07/27 04:13:48, Keith wrote:
> > To me, having done numerical work, the chain of functions you use, int(
ceil(
> > -log2(x))), is easily recognizable as "find the smallest n so that 1 / 2^n
<=
> > x".
> > 
> > While log2 is inexact, the results when x is a power of 2 are exact
> > (IEEE-compliant floating-point represents integers and integer powers of 2
> > exactly) so reliably 3/4 and 2/4 both come out 1 (half/minim rest).
> 
> log2 is a transcedental operation, it is not guaranteed to be exact.  logb
would
> be (it just returns the exponent from the floating point representation).
> 
> Anyway, it seems like log2 is not universally available, so we should avoid
it. 
> The same is likely true for logb.

Why not just an array that acts as a log table?

int foo[10] = {1,2,4,8,16,32,64,128,256,512};

Or however long you need it to be, with a check to make sure we're in bounds. 
Then, a binary search will find the index you want.

[Keith, 2011-07-28 02:53:14]

Works good for me.


On 2011/07/27 17:49:01, MikeSol wrote:

> Why not just an array that acts as a log table?
> 
> int foo[10] = {1,2,4,8,16,32,64,128,256,512};
> 
Bertrand already has something better, walking along the (very short) list of
usable-duration-logs.

[Bertrand Bordage, 2011-07-28 12:24:09]

>
http://codereview.appspot.com/4536068/diff/37002/lily/multi-measure-rest.cc#n...
> lily/multi-measure-rest.cc:241: length = (2 << -i) / 2;
> The division by 2 changes the result.  Not that I understand too well what it
is
> supposed to be doing.

To be honest, I never understood well how this bitset operator works.
What I see is that "2 << -i" gives the same result than "2^(-i+1)".
I should have write "2 << (-i + 1)" instead of "/2"...

Besides this, I would like to remove these ugly "if"s I added yesterday, but I
don't see a better human-readable solution.

Bertrand

[benko.pal, 2011-07-28 12:32:12]

> To be honest, I never understood well how this bitset operator works.

adds trailing zeros in binary.

> What I see is that "2 << -i" gives the same result than "2^(-i+1)".
> I should have write "2 << (-i + 1)" instead of "/2"...

the +1 multiplies by 2, not divides.

perhaps the best would be 1 << -i.

now I'll really take a look at the actual patch...

p

[Bertrand Bordage, 2011-07-28 12:41:58]

> > I should have write "2 << (-i + 1)" instead of "/2"...
> 
> the +1 multiplies by 2, not divides.

Whoops, I meant -(i + 1)...

> perhaps the best would be 1 << -i.

Thanks.
When I told you I never understood "<<", I wasn't kidding!
An update will follow this comment.

Bertrand

[benko.pal, 2011-07-28 12:46:53]

hi Bertrand,

I started at the patch but it's quite difficult for now,
I hope I'll have enough time in the evening.  till then
could you tell me whether usable-duration-logs is
ordered?  is it a range or can it have holes?

thanks,
p

[Bertrand Bordage, 2011-07-28 13:01:06]

It doesn't need to be ordered.
It can have holes, but there's a small issue with this for now:
Church rests are only looking for maximum and minimum values. You can therefore
find longas in a church rest if you set usable-duration-logs to '(0 -1 -3).
I don't know whether it's better to keep this system or to stick to
usable-duration-logs.
This would be logical to stick to the list, but church rests are something
special that doesn't behave the same way than uncompressed multi-measure rests. 
Maybe a separated list for church rests would be the best solution...

Thanks,
Bertrand

[benko.pal, 2011-07-31 09:16:02]

hi Bertrand,

the patch is correct, AFAICS; see some minor improvements below.


minor concerns (which shouldn't delay acception):
1. about the very existence of usable-duration-logs -
   ok, it's generic, but who uses this genericity?
   is it not always (0 -1 -2 -3)?
   is it not always a range with lower end -3?
   is it not always a range?
2. comments, more descriptive names, e.g.
   round_up instead of round in calc_measure_duration_log;
   measure_count instead of measures (and l),
   symbol_count instead of count in church_rest

http://codereview.appspot.com/4536068/diff/37004/lily/multi-measure-rest.cc
File lily/multi-measure-rest.cc (right):

http://codereview.appspot.com/4536068/diff/37004/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:163: closest_usable_duration_log =
minimum_usable_duration_log;
I think these two lines can be moved after the loop

http://codereview.appspot.com/4536068/diff/37004/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:257: int mdl = calc_measure_duration_log (me);
move this before the loop

http://codereview.appspot.com/4536068/diff/37004/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:268: k = mdl + i;
I'd write lines 254-268 like

    /*
      find the longest usable rest fitting into l:
      k identifies a canditate by its duration-log,
      length is its duration (in mdl units)
    */
    int k = longest_church_rest;
    int length = 1 << (mdl - k);
    for (; length > l; ++k)
      length >>= 1;

    l -= length;

[Bertrand Bordage, 2011-07-31 15:35:46]

Hi Pál (besides, are you Pál Benkő the chess master?)

Thanks for this nice review.

> 1. about the very existence of usable-duration-logs -
>    ok, it's generic, but who uses this genericity?
>    is it not always (0 -1 -2 -3)?
>    is it not always a range with lower end -3?
>    is it not always a range?

It can be something else.  Some recent editors are also using half and quarter
rests.
One may also want to only have whole rests, so I guess we don't have to
hard-code -3.
I don't honestly know if this has to be range or if bounds would be enough.
I solved the issue I mentioned in the last comment, so that church rests only
pick duration logs from usable-duration-logs.

I applied the other changes.

Thanks again,
Bertrand

[pkx166h, 2011-07-31 20:10:30]

Passes Make and there is a reg test difference which looks ok. I created a
tracker

http://code.google.com/p/lilypond/issues/detail?id=1794

so people can see this but also because this has been going on for a while and
the tracker will at least keep this on people's radars.

[pkx166h, 2011-07-31 22:08:33]

On 2011/07/31 20:10:30, J_lowe wrote:
> Passes Make and there is a reg test difference which looks ok. I created a
> tracker
> 
> http://code.google.com/p/lilypond/issues/detail?id=1794
> 
> so people can see this but also because this has been going on for a while and
> the tracker will at least keep this on people's radars.

Actually I made a mistake in my reg check process (missed the make after the
patch apply). It now looks as Bertrand says it should.

[benko.pal, 2011-08-01 11:31:41]

hi Bertrand,

(I'm not a grand-master, :( )

good that you caught the problem with hole in usable-duration-logs, but, I
think, that makes more comment necessary, see below.

all reviewers: is there a convention about using assertions in C++ code?

http://codereview.appspot.com/4536068/diff/55001/lily/multi-measure-rest.cc
File lily/multi-measure-rest.cc (right):

http://codereview.appspot.com/4536068/diff/55001/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:255: find the longest usable rest fitting into l:
fitting into measure_count

http://codereview.appspot.com/4536068/diff/55001/lily/multi-measure-rest.cc#n...
lily/multi-measure-rest.cc:257: length is its duration (in mdl units)
append to the comment something like

      note: if length were zero on exit, the while-loop wouldn't terminate,
      but this shouldn't happen since mdl is known to be in duration_logs_list

and perhaps (what is LP convention?) add an

    assert(length > 0);

after the for-loop.

[Bertrand Bordage, 2011-08-04 15:20:16]

Hi,

I think I found a proper way to calculate church rests.
I also updated so that it applies on the latest git HEAD.

Bertrand.