Re: Issue 3174: Implement \accidental markup and \pitched command for transposable accidental markups (issue 7323060)
Reviewers: Trevor Daniels, thomasmorley65,
Message:
On 2013/02/13 23:27:55, thomasmorley65 wrote:
On 2013/02/13 22:56:30, Trevor Daniels wrote:
I definitely prefer the alternative you suggest in para 3.
You would not then need the warning, which is only necessary due to
the
complicated 2-stage interface.
I'd second.
Here is the deal: I can rename the current \pitched to \withPitch
which is quite in line with \withMusicProperty.
\withPitch/\accidental is a basic mechanism for passing transposition
information from music to markups.
The questions are
a) is this mechanism sufficient for pitched ornaments?
b) are there useful applications apart from pitched ornaments?
If both answers are no, this approach does not lead anywhere. With
regard to question a), the main question I see here is should this be
tied into the accidental rule system. If it isn't, it remains the
user's task to properly distribute accidentals and cautionaries. Now
fortunately, the question in-scale or out-of-scale remains
constant under _chromatic_ transposition (and modal transposition is
quite less complete), so while this is cumbersome, it still tackles
the basic problem of transposability. It also begs the question
whether we should not just be passing pitch but also
force-accidental and cautionary and have the \accidental markup
heed them.
Actually, force-accidental does not make much sense since setting it
to #f would mean having to check whether the current accidental rules
would demand an accidental, and that is a check that is, I think,
beyond the design of this feature as long as should this be tied into
the accidental rule system can be answered with no.
So at any rate, the question this patch answers is the question can
we pass information about transposed accidentals through to markups,
and the answer is yes. Is this a good interface to pitched
ornamentations has been answered no, and can it be a good building
block for a good interface to pitched ornamentations will depend on
what we agree on being a good interface to pitched ornamentations.
So the message is we can do, but we still need to figure out _what_
we can do. Proposals?
Description:
Issue 3174: Implement \accidental markup and \pitched command for
transposable accidental markups
This is a building block for creating pitched ornaments behaving
sensibly under transposition.
It would be arguable to define \pitched differently so that it works
only on ornaments and does the stacking itself, like
c^\pitched\trill des
and instead give the current version of \pitched a name like \pitchify.
As it stands, here are the implemented meanings:
\accidental markup:
Draw an accidental based on the @var{pitch} property of the causing
event. This is typically set in music using the (transposable)
@code{\pitched} command, like
@lilypond[verbatim,quote]
melody = \relative c'
{ \key c\major c2^\trill^\pitched des -\markup \accidental d! }
\new Staff { \melody }
\new Staff { \transpose c' e' \melody }
@end lilypond
\pitched music function: arguments pitch and music:
Give @var{music} a pitch of @var{pitch}.
Please review this at https://codereview.appspot.com/7323060/
Affected files:
M ly/music-functions-init.ly
M scm/define-markup-commands.scm
Index: ly/music-functions-init.ly
diff --git a/ly/music-functions-init.ly b/ly/music-functions-init.ly
index
c063b9585da50b8d9325f73aab5ebb4c0c45cd6e..f23e19a7c00d007793417ac490c23fe5063123bf
100644
--- a/ly/music-functions-init.ly
+++ b/ly/music-functions-init.ly
@@ -963,6 +963,13 @@ partial =
'Timing)
'Score))
+pitched =
+#(define-music-function (parser location pitch music)
+ (ly:pitch? ly:music?)
+ (_i Give @var{music} a pitch of @var{pitch}.)
+ (set! (ly:music-property music 'pitch) pitch)
+ music)
+
pitchedTrill =
#(define-music-function
(parser location main-note secondary-note)
Index: scm/define-markup-commands.scm
diff --git a/scm/define-markup-commands.scm b/scm/define-markup-commands.scm
index
7dd0f967ed26bea6631292e2832ad56efd5e9d98..23c2ae239e9bb83f745bdfed51ccc902bd61fd5b
100755
--- a/scm/define-markup-commands.scm
+++ b/scm/define-markup-commands.scm
@@ -2750,6 +2750,34 @@ the possible glyphs.
glyph))
+(define-markup-command (accidental layout props)
+ ()
+ #:category music
+ #:properties ((cause))
+ Draw an accidental based on the @var{pitch} property of the causing
+event. This is typically set in music using the (transposable)
+@code{\\pitched} command, like
+
+@lilypond[verbatim,quote]
+melody = \\relative c'
+ { \\key c\\major c2^\\trill^\\pitched des -\\markup \\accidental d! }
+\\new Staff { \\melody }
+\\new Staff { \\transpose c' e' \\melody }
+@end lilypond
+ (let* ((event (cond ((ly:grob? cause) (event-cause cause))
+ ((ly:stream-event? cause) cause)
+ (else #f)))
+ (pitch (and event (ly:event-property event 'pitch
+(if (ly:pitch? pitch)
+
Re: Issue 3174: Implement \accidental markup and \pitched command for transposable accidental markups (issue 7323060)
Here is the deal: I can rename the current \pitched to \withPitch which is quite in line with \withMusicProperty. \withPitch/\accidental is a basic mechanism for passing transposition information from music to markups. The questions are a) is this mechanism sufficient for pitched ornaments? I don't think so. For example, mordents sometimes need *two* accidentals, one written above and another one below. But maybe this can easily fixed by allowing a second, optional argument. b) are there useful applications apart from pitched ornaments? Good question. AFAIK, only ornaments need that. So the message is we can do, but we still need to figure out _what_ we can do. Proposals? I'm currently lacking inspiration, sorry. Werner ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Patch meister position still vacant!!!
Graham Percival gra...@percival-music.ca writes: On Tue, Feb 12, 2013 at 09:26:45AM +0100, David Kastrup wrote: James pkx1...@gmail.com writes: Anyway, assuming that no one else offers, I'll take a look and make a start of it on Wednesday. I certainly think you a good fit for that position. The problem is that you are a good fit for a _lot_ of positions, and, for example, make very good and thorough manual patch reviews. I echo those concerns. It should take 15-30 minutes a week. It's a purely mechanical secretary job. Trivial for a non-developer to learn. I disagree with that characterization which does not do justice to job Colin has been doing and the rather thorough start James had made. It is not a purely mechanical job since it involves reading the reviews and judging when concerns have been addressed and when not and what to best do about that. The advantage of the job over the Patchy review job is that it does not involve installing or compiling any software. At any rate, I've been offloading that job because I was getting overwhelmed. Now part of the reason is that I don't necessarily have the best personality to deal with that kind of responsibility and conflict of interest. But part of it _is_ that it definitely is a non-trivial amount of work and judgment, and that is why Colin needed a sabbatical from it. I'll pitch in with Patchy reviews somewhat to compensate, but I think that James is currently taking more of a load than I think good in the long term. -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Add Kievan ligature engraver (issue 7301097)
Reviewers: , Message: Please review. Description: Add Kievan ligature engraver Creates support for melismas (ligatures) in Kievan notation via the use of a ligature engraver. Please review this at https://codereview.appspot.com/7301097/ Affected files: M lily/coherent-ligature-engraver.cc A + lily/include/kievan-ligature.hh A lily/kievan-ligature-engraver.cc A + lily/kievan-ligature.cc M lily/ligature-engraver.cc M ly/engraver-init.ly M scm/define-grobs.scm ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Patch meister position still vacant!!!
hello, On 14 February 2013 11:33, David Kastrup d...@gnu.org wrote: Graham Percival gra...@percival-music.ca writes: On Tue, Feb 12, 2013 at 09:26:45AM +0100, David Kastrup wrote: James pkx1...@gmail.com writes: Anyway, assuming that no one else offers, I'll take a look and make a start of it on Wednesday. I certainly think you a good fit for that position. The problem is that you are a good fit for a _lot_ of positions, and, for example, make very good and thorough manual patch reviews. I echo those concerns. It should take 15-30 minutes a week. It's a purely mechanical secretary job. Trivial for a non-developer to learn. I disagree with that characterization which does not do justice to job Colin has been doing and the rather thorough start James had made. It is not a purely mechanical job since it involves reading the reviews and judging when concerns have been addressed and when not and what to best do about that. The advantage of the job over the Patchy review job is that it does not involve installing or compiling any software. At any rate, I've been offloading that job because I was getting overwhelmed. Now part of the reason is that I don't necessarily have the best personality to deal with that kind of responsibility and conflict of interest. But part of it _is_ that it definitely is a non-trivial amount of work and judgment, and that is why Colin needed a sabbatical from it. I'll pitch in with Patchy reviews somewhat to compensate, but I think that James is currently taking more of a load than I think good in the long term. I know that Colin trawled all the email lists whereas I am only looking at Dev and Bug (which I am subscribed to) it does need some judgement, but only to not offend by pushing a patch to 'needs work' mistakenly or letting a patch through to countdown because you miss a response on an email list and not on Rietveld or the Tracker. Testing patches obviously takes time but I am very used to it now and the recent python scripts have helped a *lot* - fire and forget, mostly the CPUs do all the work, and apart from the odd case of a failing make check (where it can be tricky to fathom the multi-processes in the log files for specific errors to help analyse the issue) it's pretty straight forward now, and as long as people don't mind downloading the FTP links for those reg diffs that are significant in size (I can't think of a better way to do that) that I cannot attach to the Tracker, then it's not that big a load as you might think. While Colin did his patch review on specific days, it's easier (for me) to do mine on a 3 day roll regardless of the Day of the week. Just shifting things up/down/off the list. At the moment I can handle this and I see no reason that I won't be able to in the future, the only issue for the team is when I am away - on holiday for instance - patchy merge can keep running as that just ticks away in the background, In the end it is still better that developers develop and people like me do the boring but necessary admin stuff. Let's see how it goes. James ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Patch meister position still vacant!!!
James pkx1...@gmail.com writes: In the end it is still better that developers develop and people like me do the boring but necessary admin stuff. I have no beef with that, except when the only exemplar of people like you seen in the wild happens to be you. I've been in the well, seems nobody is better qualified for that than you trap for at least half a year in LilyPond before I realized that I would not be able to do anything _but_ LilyPond if I kept feeling responsible for everything that I am good at. I am still stuck in that trap, but since I got people to pay my bills for me while I am stuck in it, I am working on LilyPond only at the expense of my work on LilyPond. Take care. -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Caches the interior skylines of vertical axis groups and systems. (issue 7185044)
I've not really reviewed everything here, just highlights. Regarding the commenting problems: it is important for the reviewer or maintainer or rereader to get the gist of the story. Much of the LilyPond codebase has been written with a total disregard to people not present during the writing. For that reason, it is not a useful metric for writing LilyPond code that is supposed to get reviewed and maintained by people other than the original author. If a review is to make sense, I think Han-Wen would likely understand this is not enough since the reviewers are different from Han-Wen, and we want more people than just you and him to be able to maintain this code in future. The LilyPond code base has a maintainability problem due to historic reasons, and we need to move away from that legacy rather than adding to it. Only part of this review is actually specific to this particular patch. A lot is rather trying to bring across some general advice to coding and commenting practice. https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-positioning.ly File input/regression/tuplet-number-outside-staff-positioning.ly (right): https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-positioning.ly#newcode15 input/regression/tuplet-number-outside-staff-positioning.ly:15: \override TupletBracket.Y-offset = #ly:side-position-interface::calc-outside-staff-y-offset This is indicating a fundamental design problem: this override is not related to the topic of the regtest. If it is necessary for getting the desired result, it will be necessary in a whole _lot_ of user-created situations. But it is not an easily user-accessibly override, and it is not documented in normal user documentation. This suspiciously looks like tampering with unrelated regtests in order to mask fundamental problems from review. Can you explain why this is not the case? https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-priority.ly File input/regression/tuplet-number-outside-staff-priority.ly (right): https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-priority.ly#newcode13 input/regression/tuplet-number-outside-staff-priority.ly:13: \override TupletNumber.Y-offset = #ly:side-position-interface::calc-outside-staff-y-offset See above comment. If outside-staff-priority ceases to work, a lot of user-level documentation would warrant adaption. https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc File lily/axis-group-interface.cc (right): https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc#newcode595 lily/axis-group-interface.cc:595: Axis_group_interface::staff_priority_less (Grob *const g1, Grob *const g2) Is there a reason you turn this into member functions? https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc#newcode713 lily/axis-group-interface.cc:713: Axis_group_interface::prepare_for_outside_staff_calculations (Grob *me) There is no description anywhere what this preparation will entail, what it looks at, and what it does. Is this used even more than once? https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc#newcode755 lily/axis-group-interface.cc:755: Axis_group_interface::sort_outside_staff_elements (Grob *me, vectorGrob * elements) This looks like it does a whole lot more than just sorting some elements. What does it do? Why does it do that? https://codereview.appspot.com/7185044/diff/32003/lily/include/axis-group-interface.hh File lily/include/axis-group-interface.hh (right): https://codereview.appspot.com/7185044/diff/32003/lily/include/axis-group-interface.hh#newcode69 lily/include/axis-group-interface.hh:69: static bool staff_priority_less (Grob *const g1, Grob *const g2); All those were previously staic functions inside of axis-group-interface.cc, and thus limited to that file. Now you have made them part of the class definition, and in addition you have turned then into a _public_ part of the class definition, suggesting that they are part of the external interface of the class. Why? https://codereview.appspot.com/7185044/diff/32003/lily/include/directional-element-interface.hh File lily/include/directional-element-interface.hh (right): https://codereview.appspot.com/7185044/diff/32003/lily/include/directional-element-interface.hh#newcode26 lily/include/directional-element-interface.hh:26: // what is the advantage not having these two as STATICs of GROB -- jcn these three it would appear now. And the question seems valid. https://codereview.appspot.com/7185044/diff/32003/lily/include/side-position-interface.hh File lily/include/side-position-interface.hh (right): https://codereview.appspot.com/7185044/diff/32003/lily/include/side-position-interface.hh#newcode45 lily/include/side-position-interface.hh:45: static Real
Re: let beam thickness depend on line thickness (fix 3173) (issue 7312091)
https://codereview.appspot.com/7312091/diff/1/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): https://codereview.appspot.com/7312091/diff/1/scm/define-grob-properties.scm#newcode93 scm/define-grob-properties.scm:93: (beam-thickness ,number-pair? Beam thickness. It is the sum You are confusing sum and pair in this comment. https://codereview.appspot.com/7312091/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Patch meister position still vacant!!!
- Original Message - From: David Kastrup d...@gnu.org To: lilypond-devel@gnu.org Sent: Thursday, February 14, 2013 6:38 PM Subject: Re: Patch meister position still vacant!!! James pkx1...@gmail.com writes: In the end it is still better that developers develop and people like me do the boring but necessary admin stuff. I have no beef with that, except when the only exemplar of people like you seen in the wild happens to be you. I think there's quite a number of us. I count myself there. Both Colins do/did great work keeping the admin ticking over, without delving into the C++/scheme. There's others too numerous to mention. -- Phil Holmes ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Patch meister position still vacant!!!
Phil Holmes m...@philholmes.net writes: - Original Message - From: David Kastrup d...@gnu.org To: lilypond-devel@gnu.org Sent: Thursday, February 14, 2013 6:38 PM Subject: Re: Patch meister position still vacant!!! James pkx1...@gmail.com writes: In the end it is still better that developers develop and people like me do the boring but necessary admin stuff. I have no beef with that, except when the only exemplar of people like you seen in the wild happens to be you. I think there's quite a number of us. Why then lay several jobs on one? -- David Kastrup ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Patch meister position still vacant!!!
On Thu, Feb 14, 2013 at 9:43 PM, David Kastrup d...@gnu.org wrote: Phil Holmes m...@philholmes.net writes: From: David Kastrup d...@gnu.org I have no beef with that, except when the only exemplar of people like you seen in the wild happens to be you. I think there's quite a number of us. Why then lay several jobs on one? Uh, what about pigeonhole principle? [1] If we have n jobs (and in case of Lily n is obviously 1) and m people, and m n, then someone will have 1 job. Of course we could argue whether the distribution is even and so on, but i really think that's a dangerous topic. I think there is a greater chance of running into some misunderstanding than coming to constructive results. cheers, janek [1] http://en.wikipedia.org/wiki/Pigeonhole_principle ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Patch meister position still vacant!!!
- Original Message - From: David Kastrup d...@gnu.org To: lilypond-devel@gnu.org Sent: Thursday, February 14, 2013 8:43 PM Subject: Re: Patch meister position still vacant!!! Phil Holmes m...@philholmes.net writes: - Original Message - From: David Kastrup d...@gnu.org To: lilypond-devel@gnu.org Sent: Thursday, February 14, 2013 6:38 PM Subject: Re: Patch meister position still vacant!!! James pkx1...@gmail.com writes: In the end it is still better that developers develop and people like me do the boring but necessary admin stuff. I have no beef with that, except when the only exemplar of people like you seen in the wild happens to be you. I think there's quite a number of us. Why then lay several jobs on one? If you want a job done well, ask a busy man. -- Phil Holmes ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Issue 3174: Implement \accidental markup and \pitched command for transposable accidental markups (issue 7323060)
On 2013/02/14 09:21:07, dak wrote: So the message is we can do, but we still need to figure out _what_ we can do. Proposals? Well, currently I don't have a good idea. Though, some time ago Arnold posted his approach on -user: http://lists.gnu.org/archive/html/lilypond-user/2012-06/msg00278.html direct link to his code: http://old.nabble.com/file/p33991423/pitchedArticulations.ly Perhaps this might be useful, I hadn't a closer look, though. https://codereview.appspot.com/7323060/ ___ lilypond-devel mailing list lilypond-devel@gnu.org https://lists.gnu.org/mailman/listinfo/lilypond-devel
Re: Caches the interior skylines of vertical axis groups and systems. (issue 7185044)
Thank you for the _excellent_ review. This is _exactly_ the type of stuff I need. https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-positioning.ly File input/regression/tuplet-number-outside-staff-positioning.ly (right): https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-positioning.ly#newcode15 input/regression/tuplet-number-outside-staff-positioning.ly:15: \override TupletBracket.Y-offset = #ly:side-position-interface::calc-outside-staff-y-offset This is indicating a fundamental design problem: this override is not related to the topic of the regtest. If it is necessary for getting the desired result, it will be necessary in a whole _lot_ of user-created situations. But it is not an easily user-accessibly override, and it is not documented in normal user documentation. This suspiciously looks like tampering with unrelated regtests in order to mask fundamental problems from review. Can you explain why this is not the case? Your question gets to the core of the logic of the patch, so I'll explain it and then people can comment upon how that links up with this regtest. In LilyPond, about 85% of grob properties are set as the result of the evaluation of a callback or the use of a rote value. About 15% are set via calls to set_property and translate_axis (the latter of which only controls Y and X offset). The top-down approach to property setting (i.e. a VerticalAxisGroup controlling the Y-offset of its elements, which is currently the case for outside staff positioning) poses the problem that once a value is cached, it is very difficult to run calculations again for that value once more precise information is available. With the bottom-up approach (grobs reporting their properties to their collecting grobs), however, this is easy and is done all over the code base. For example, in separation-item.cc, approximations of heights are used in Separation_item::boxes to do horizontal spacing. I am trying to get LilyPond to a point where it does vertical spacing the same way. This will allow for much better approximation of distances between staves containing cross-staff grobs. If this is the case that grobs' offsets will be controlled by callbacks and not by uber-grobs like VerticalAxisGroup calling translate_axis a bunch of times, then grobs that are going to be placed outside the staff need to have their Y-offset function reflect that. Now, LilyPond, for various callbacks, other properties must be set for those callbacks to make sense. For example, if I do: \override NoteHead #'stencil = #ly:text-interface::print Nothing will happen. But if I do: \override NoteHead #'text = #foo \override NoteHead #'stencil = #ly:text-interface::print The NoteHead will be printed as foo. This is exactly what we're seeing in the regtest tuplet-number-outside-staff-positioning.ly. A callback is set for Y-offset that allows the grob to be positioned outside the staff. But, just as the text interface needs to know what text to print, the side-position-interface needs to know what outside-staff-priority to use to place the grob. Thus the use of two overrides instead of one. A music function could be done in the form of: \addOutsideStaffPriorityToGrob #'TupletBracket #100 that rolls the two overrides into one. This is a UI issue about which I have not thought yet but that absolutely deserves attention. For those who are more adept at UI than I, I'm very interested in your ideas. Furthermore, how could this be documented to make sense to the other user. For me, the sentence some overrides require supplemental overrides to kick in seems kind of abstract and geeky. How can this be communicated? https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-priority.ly File input/regression/tuplet-number-outside-staff-priority.ly (right): https://codereview.appspot.com/7185044/diff/32003/input/regression/tuplet-number-outside-staff-priority.ly#newcode13 input/regression/tuplet-number-outside-staff-priority.ly:13: \override TupletNumber.Y-offset = #ly:side-position-interface::calc-outside-staff-y-offset See above comment. If outside-staff-priority ceases to work, a lot of user-level documentation would warrant adaption. I agree, but this would only be the case for grobs not already using ly:side-position-interface::y-aligned-side. Currently, every grob using outside-staff-priority is using this function as a callback. So the only thing that users will need to be made aware of is the effect of this change on overrides. What would be a good way to communicate that? https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc File lily/axis-group-interface.cc (right): https://codereview.appspot.com/7185044/diff/32003/lily/axis-group-interface.cc#newcode595 lily/axis-group-interface.cc:595:
