Re: Issue 3061 (issue 7033045)

[dak]

On 2013/01/01 22:05:58, zefram_fysh.org wrote:
> mailto:address@hidden wrote:
> >All this seems far too complex.  Why not use ly:duration-length and
just
> >divide the respective values of ly:moment-main?  No need to fiddle
with
> >counting dots and shift factors manually.

> That's a valid approach, but doesn't win quite as much as you'd think.
> In the most obvious form, it would change the tgt-nrep binding to
> something like this (untested):

>          (tgt-nrep (/ (ly:moment-main (ly:duration-length totaldur))
>                       (ly:moment-main (ly:make-duration tremtype-log 0
>                                         (ly:duration-scale totaldur)))))

> It does involve fewer function applications (6 instead of 11).  But on
> the downside it'll break if the note has been scaled by a factor of
> zero.

Excellent point.  Of course, it begs the question how to articulate
_anything_ in the presence of scaled durations.

> Fixing that increases the complexity:

>          (tgt-nrep (/ (ly:moment-main
>                         (ly:duration-length
>                           (ly:make-duration (ly:duration-log totaldur)
>                                             (ly:duration-dot-count totaldur)
>                                             1)))
>                       (ly:moment-main (ly:make-duration tremtype-log 0 1))))

> Eight function applications, still fewer than my original, but it has
> more long function names (and so takes more text lines given a
reasonable
> length limit), and doesn't avoid the need for the code to know about
dot
> counts.

You are counting characters here.  That's not a useful measure of
complexity.

> It doesn't have to *understand* dot counts, though, so that's
> a bit of a win.

It is not just "a bit" of a win.  The code for expanding and
displaying tremolo dot counts have been at odds with the execution for
the longest amount of time (check out issue 2602
<URL:http://code.google.com/p/lilypond/issues/detail?id=2602>).  Not
having to interpret the contents, even if it takes just 15 characters
in APL, is a big win.

> Also, the de-scaled version of totaldur can be brought
> out as a separate binding and then reused for the "non-integer
tremolo"
> warning message.

The question is whether this is the most useful approach.  Other
possibilities would be to give ly:duration-length an optional argument
for overriding the scale factor to an arbitrary value, or make an
extra function ly:visual-duration-length that ignores the scale factor.

> I think what would *really* make this code more readable is a bunch of
> library functions covering common subexpressions that are likely to
come
> up in music processing.  For example:

>      (define (duration-dot-factor dotcount)
>         (/ (1- (ash 2 dotcount)) (ash 1 dotcount)))

(define (duration-dot-factor dotcount)
  (- 2 (/ (ash 1 dotcount))))

Not sure whether it is clearer, but at least it makes glaringly
obvious that the factor does not go above 1, a frequent problem with
bad dot-count expansion expressions.  It also makes reasonably clear
that a dotcount of 0 corresponds to a factor of 1 without
special-casing.

>      (define (duration-descale dur)
>         (ly:make-duration (ly:duration-log dur) (ly:duration-dot-count dur)
1))

> I've written some much larger chunks of LP Scheme code that I ought to
> trawl for opportunities like this.

duration-rescale with an optional argument defaulting to 1 maybe?  Or
would that never be useful?

> Also, of some relevance to this code, the system of "moment" objects
> is a bit cumbersome.  I can see why you need the two-part structure
> for aligning music columns in the output, but it looks like most
> things using moment structures only use the main part, making it
> just an unnecessary wrapper around rationals.

It is a _necessary_ wrapper around Rational, an internal C++ data type
predating the availability of rational numbers in Guile.

> The arithmetic on them hasn't been thought out properly:
> ly:moment-div returning a moment is just ridiculous.

It is just a consequence of ly:moment-div being a substitute for
(/ <Rational> <Rational>) -> <Rational>.

> It'd be nice if moments were restricted to their proper role in
> typesetting, and everything else just used rationals.

Moments with grace timing don't have useful context-independent
arithmetic properties, so it would make sense to obliterate them
altogether and just work with rationals everywhere.

Also there is no point in having a separate C++ type "Rational", it is
just cause for trouble.  Replacing it with SCM is not entirely trivial
since rational numbers are not immediate entities and thus need to be
marked during garbage-collection, also affecting every C++ type
containing rationals.

> So, getting back to the subject, let me know if you'd
> like me to produce a complete tremolo patch based on the variant
> that I described above.  Or a patch factoring out some of the code
> into a library of wider utility.

"a" library of wider utility is not really required: we already have
lily-library.scm and music-functions.scm (with no really clear
criteria about when something should be put into either).

The existing library, including the available provisions of
ly:... exported functions, is badly documented, inconsistent, not
described in any part of Documentation, partly duplicates existing
functionality in the Guile libraries, and has partly never been used
or updated.

If you plan to have a go at it, you have my blessings.  You'll likely
get a lot of feedback of mixed quality and conflicting opinions, but
there is certainly a lot of progress in that area to be made.

Oh, with regard to obbliterating the C++ type Rational: I have some
branch flying around called "unrational" where I try doing just that.
It is just that the work in this branch, done alphabetically, has
stalled somewhere in the middle of the letter "A" or "B".

https://codereview.appspot.com/7033045/