Issue 3061

Issue 3061

articulate TremoloEvent

Expand TremoloEvent to repeated notes, in the unfolding stage of
articulation.

[dak, 2013-01-01 20:18:25]

https://codereview.appspot.com/7033045/diff/1/ly/articulate.ly
File ly/articulate.ly (right):

https://codereview.appspot.com/7033045/diff/1/ly/articulate.ly#newcode455
ly/articulate.ly:455: ((tremtype (ly:music-property (car trem) 'tremolo-type))
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.

[zefram_fysh.org, 2013-01-01 22:05:58]

dak@gnu.org 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.
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.  It doesn't have to *understand* dot counts, though, so that's
a bit of a 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.  I'd be happy for such a variant of my patch to be used
in place of the original; I see no compelling grounds to go either way.

Generally, the complexity of the code I submitted comes from being
robust in the face of possibly uncooperative data.  I'm thinking about
situations such as where the overall duration is not an exact multiple
of the nominal tremolo duration, or the music has been subjected to time
scaling, possibly by awkward factors.

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-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.

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.  The arithmetic on them hasn't been thought out
properly: ly:moment-div returning a moment is just ridiculous.  It'd be
nice if moments were restricted to their proper role in typesetting,
and everything else just used rationals.  (With grace note articulation
through \articulate working, I think it'd be viable for basic MIDI output
to totally ignore grace notes, just as it ignores most articulations,
so even it wouldn't need moment structures.  But I digress.)

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.

-zefram

[dak, 2013-01-02 08:39:13]

On 2013/01/01 22:05:58, zefram_fysh.org wrote:
> mailto:dak@gnu.org 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".

[zefram_fysh.org, 2013-01-02 11:15:04]

dak@gnu.org wrote:
>Excellent point.  Of course, it begs the question how to articulate
>_anything_ in the presence of scaled durations.

The result will presumably end up as zero-duration silence, but I'd
rather it were *properly articulated* zero-duration silence.  Well,
seriously, you could detect the zero scale factor as a special case and
return null music for it, but (a) that just puts the additional code in a
different place and (b) it's a nasty exception that makes \articulate less
algebraically well behaved.  Consistently emitting a scaled articulation,
whatever the scale factor, means that when we look at how anything else
handles the zero-scale case we don't need to worry specifically about
how it'll interact with \articulate.

>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

Ah, interesting.  OK, we'll keep that knowledge localised.

>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'd rather not overload existing functions.  visual-duration-length
(I'd call it duration-visual-length) is sensible, and goes alongside my
duration-descale, which could be correspondingly called visual-duration.

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

Neater than I had it.

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

I don't see a need for overriding the scale factor to anything other
than 1.  If the need arises then the circumstances will give us some
clue as to how best to present the operation.

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

Makes sense, though a pity that the rational wrapper also got lumped
with the grace-alignment job.  I wasn't aware that Guile had ever lacked
proper rationals.  (This is actually the first project on which I've
used Guile, and the first significant use I've made of Scheme.)

I suppose that history also explains why so much of the code processes
rationals as an unboxed numerator/denominator tuple.  That's another thing
to eliminate now that there are proper rationals.  The only fractions
that need to be handled that way are time signatures, which are not
merely arithmetical fractions.

>"a" library of wider utility is not really required: we already have
>lily-library.scm and music-functions.scm

Right, I was thinking this stuff would probably belong in
music-functions.scm.  I'll produce a revised patch that factors some of
the logic out this way.

-zefram

[zefram_fysh.org, 2013-01-02 20:08:29]

Attached patch goes on top of my original tremolo articulation patch.
I think it would be sensible for them to remain as separate git commits,
because this new one is really a spin-off taking the tremolo work
in a new direction, rather than changing anything about the tremolo
articulation itself.

-zefram

[pkx166h, 2013-01-03 23:29:28]

Hello

On 2 January 2013 20:08, Zefram <zefram@fysh.org> wrote:
> Attached patch goes on top of my original tremolo articulation patch.
> I think it would be sensible for them to remain as separate git commits,
> because this new one is really a spin-off taking the tremolo work
> in a new direction, rather than changing anything about the tremolo
> articulation itself.
>
> -zefram

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

James

[pkx166h, 2013-01-10 21:44:03]

author  Zefram <zefram@fysh.org>
 Sun, 23 Dec 2012 09:52:48 +0000 (09:52 +0000)
committer  James Lowe <pkx166h@gmail.com>
 Thu, 10 Jan 2013 21:41:33 +0000 (21:41 +0000)
commit51146a320837b3d2f673c8ce0493e33038babc18