Hey, lilypond-devel complained about the recipient list of this message, so I am resending it. Sorry for those who are receiving it for the second time.
---------- Forwarded message ---------- Hello everyone, First of all, thanks for the extensive comments made on this. I will try to address the major topics in this mail. Sorry if I miss anything. ======= 1. Issue header > Nitpick: if this were called something like "change internal pitch > representation" in the first line of the patch, I would have looked > earlier. From a casual glance at my mailbox, it looked like it would > be a simple bugfix patch for issue 1278. I should remind you that this patch was at first only casually posted as a background for a discussion I started by the end of the last year, see http://lists.gnu.org/archive/html/lilypond-devel/2010-12/msg00726.html I am taking this review as a continuation of that discussion. So, for those of you who did not follow that, I will give relevant pointers below. For now, I think you really should check the attachments in http://lists.gnu.org/archive/html/lilypond-devel/2010-12/msg00730.html I will occasionally make references to towards123, which is attached in http://lists.gnu.org/archive/html/lilypond-devel/2010-12/msg00729.html ----- 2. int vs Rational > Why not use a sequence of Rationals (rather than ints) to represent > the alteration? > If we use rational numbers, we can maintain backward compatibility. > The re-scaling of the alterations is interesting but > unnecessary. They could still be rationals. That is a very reasonable idea. Its main advantage would be, as pointed, in more robust backwards compatibility, as well as ease of implementation. If you decide for that, I can work on a new patch. Further considerations of mine might be found below question 1.1 in http://lists.gnu.org/archive/html/lilypond-devel/2010-12/msg00729.html and after the last quote in http://lists.gnu.org/archive/html/lilypond-devel/2010-12/msg00737.html ----- 3. Transposition > you have to set a rule that (0 . 1) + (0 . 1) = (1 . 0). No, we don't. Transposing c to cis takes cis to cisis, not to d. If you are not using arrow notation, you can define a quarter tone to be (1 . 0). Also, see the next topic, for further discussion. ----- 4. Normalisation Many objections regarding transposition in my current patch, or in LilyPond's current behaviour, are related to what I am calling normalisation. This is what may cause musical transposition to look complicated, it really is not. My considerations on the subject can be found in Section 3 of towards123. ----- 5. Scale 1 > I think that the arguments to make-scale should not have implicit steps > (which they currently do) Steps _mean_ position on Scale, which in its turn means position in the staff. I assume this to uniquely determine one of the coefficients in the group representation of the pitch. The vector argument to make-scale then just specifies what alteration is attached to each position when no accidental is added to it. This well models every notation system I have come across. Besides, a notation system in which this did not work would either need some specific algorithm to determine the staff position of a pitched note, or not work with transpositions well. ----- 6. Scale 2 > The alteration sizes aren't part of the default scale. > It's counter intuitive to set them in that function. Firstly, the default scale effectively is just the static part of the C++ Pitch class, except when you instantiate a Pitch, then reset the default scale, and then use that Pitch (which will refer to the old scale). Secondly, see my considerations on Section 1 of towards123. ----- 7. Default arguments in ly:make-pitch > I don't see why ly:make-pitch can't take a single rational > as the alteration, and assign it as the first alteration in > the majority of cases where that's what you want. Whether we to decide for ints or for Rationals, I completely agree that make-pitch should have optional arguments. Even the alteration itself should be optional. The single reason why I chose not to enable that in this patch was to track parts of the code that would need rewriting, by making the code break until I edited them. I was particularly concerned with the commits made after my patch, that could silently invalidate it. Of course, none of this would have to be undertaken with a Rational pair/list representation. ======= If you have any further questions, or if you think that I forgot something, please write. Also: Should I write a new version of this patch using Rational instead of int? I look forward to hearing from you. Regards, Felipe _______________________________________________ lilypond-devel mailing list lilypond-devel@gnu.org http://lists.gnu.org/mailman/listinfo/lilypond-devel
