Re: Issue 754: don't transpose generic property-setting music commands (issue 7303057)

[dak]

On 2013/02/09 05:46:45, Keith wrote:
> On Fri, 08 Feb 2013 01:20:59 -0800, <mailto:address@hidden> wrote:

> > On 2013/02/08 08:40:29, dak wrote:
> >> On 2013/02/08 04:46:27, Keith wrote:
> >> > Yesterday's patch was better.
> >
> >> I beg to differ.  It did not address instrument definitions, or
manual
> > settings

> Well, we had a nice plane for \instrumentSwitch,

Did we?  The plan more or less was to put 'untransposed on it, and
that would mean that properties travelling through \instrumentSwitch
don't get any of their pitches transformed, while properties given
through \set do.  Alternatively, you just treat
instrumentTransposition, and that would still mean that setting
instrumentTransposition via \instrumentSwitch is not prone to
transposition, while setting instrumentTransposition via \set _is_
prone to transposition.

> and I did not think it wise to assume that the pitch should be
protected in
>     \set property = #(ly:make-pitch 0 -4 0)

You call it "protected".  I call it "unmolested".  There is no
inherent reason that pitches in \set/\override should be subjected to
transposition.

\transpose works on music expressions and stream events in a rather
rough manner.  Take a look at transpose_mutable in lily/music.cc.  The
rule is that any music or event property that is a pitch gets
transposed.  Every property that is named "tonic" gets a normalized
octave afterwards (ugh).  A property that is named "element" gets
recursively transposed (only if it is music, not an event), and
"elements" and "articulations" get transposed elementwise.  And if a
property is named "pitch-alist", its keys get transposed.

Now with PropertySet and OverrideProperty, the property name in the
music that we are talking about is invariably "value", so it only gets
the pitch treatment.  And if we are talking about tweaks, they appear
in arbitrary music events as a field "tweaks" always containing a
list, so nothing in tweaks gets ever transposed.

You complained about \set tonic ... not being transposable.  But
neither is \set keySignature ...  And I don't see any code which would
expect otherwise.

> I only said that patch was better; this patch is good enough.

Well, LilyPond only deserves the best.  The previous patch was most
definitely more confined, and that is its main virtue.  As a treatment
for the reported issue, this seems like the least-impact change.  In
fact, its impact was limited enough that \instrumentSwitch already was
not being covered.  So you proposed "Instrument definitions carry
context definitions for an instrument, except that
instrumentTransposition is not being transposed unlike what \set would
do."

Ugh.

The original approach with regard to the sign of
instrumentTransposition might be summarized as

    Oops, we encountered some bad side effects from set/override
    accidentally transposing values that are pitches, like
    instrumentTransposition.  What are we going to do?

    Let us change the definition of the sign of
    instrumentTransposition so that it does something really clever
    even though it is incoherent and quite useless as opposed to
    incoherent and totally useless as previously.

Now that is not cleaning up the mess but rather hanging Christmas
ornaments from it.  Some people might have gotten enamored to the
ornaments, but I don't see a reasonable way to save them.

> > Concretely: can you show an example of not-just-academical
> > LilyPond source that would be change to the worse with patch
> > applied?

> Nope.  I still encourage caution, because people use LilyPond for
> very academic projects.  Also, it is academically possible that some
> programmer will want to add a override-able property that is a pitch
> that should be transposed; he would need to find and understand this
> patch.

You are constantly working from the assumption that it is natural to
assume context-property setting commands setting pitches should be
subject to transposition.  And you list "tonic" as an example.  Except
that "tonic" needs to get _octave-normalized_ after transposition, and
\set tonic ... will _not_ achieve that.  The transposition of
instrumentTransposition is spectacularly weird and almost entirely
useless.  And so on.  Yes, there is the conceivable case of this
weirdness being _exploitable_ in a clever way for some clever purpose.

It doesn't matter.  LilyPond is still being actively maintained.  If a
feature is required, it can be implemented properly, with a proper
interface, _without_ resorting to exploits.

> > Do you consider it sensible that values set via \override are
> > transposed (but only if they are not set via a callback) while
> > values set via \tweak aren't?

> I have given it no thought.

You should before labelling one approach better than another.
instrumentTransposition turns out to be just one facet of a larger
issue, and in this case I think we are better off solving the larger
issue even though exploits for the inconsistent behavior already
exist: we have been pointed to mailing list threads demonstrating code
catered for this, like

\transpose bes c' { \transposition c' ...

for writing for an instrument with transposition bes in concert pitch.
But this particular usage will break even with the small "solution" to
the issue.  I don't know of exploits other than
instrumentTransposition, and if there are any, we'd better plug them
along with the rest.

> > Can you see any internal use of \set/\override in LilyPond source
> > (or the corresponding usage of PropertySet/OverrideProperty) that
> > would now behave inappropriately under transposition?

> No.

I am putting the patch back on countdown for 2013/02/11 then.  I am
fully aware that it is not healthy that as "Patch meister" I am judge
and jury in a process where I also happen to be defendant (or, in the
case of Mike's work, state attorney).  And I can't really pretend to
act as an impartial instance when I have very strong opinions.  But I
can't recluse myself when nobody else picks up.  So you have to make
do, or volunteer.


https://codereview.appspot.com/7303057/