Issue 2723043: Add tab-tie-follow-engraver (Closed)
|
Created:
13 years, 5 months ago by Carl Modified:
13 years, 4 months ago CC:
lilypond-devel_gnu.org Visibility:
Public. |
DescriptionAdd tab-tie-follow-engraver
Based on the Scheme engraver in Marc Hohl's pack
Patch Set 1 #
Total comments: 3
Patch Set 2 : Act only on Glissando line spanners #
Total comments: 3
Patch Set 3 : Simplify engraver (no slur or glissando) #Patch Set 4 : Change to only listen to tab-note-head events #Patch Set 5 : Get tie bounds directly in C++ #
Total comments: 7
Patch Set 6 : Remove SCM from all but slurs #
Total comments: 2
Patch Set 7 : Removed all possible scheme calls -- can't figure out one C++ call #Patch Set 8 : Switch from one property to two; decouple loops #
Total comments: 26
Patch Set 9 : Respond to comments, new print function #
Total comments: 2
Patch Set 10 : Redefine tab-note-head::print-custom-fret-label #
Total comments: 11
Patch Set 11 : Add offset to tab-note-head, delete unnecessary whiteout #Patch Set 12 : Remove tab-harmonic-engraver #Patch Set 13 : Adjust display-cautionary docstring. #Patch Set 14 : integrate tab harmonics #Patch Set 15 : Remove HarmonicParenthesesItem #Patch Set 16 : Adjust offset #MessagesTotal messages: 61
Here's a patch to Marc's tab stuff that has the engraver moved to a C++ engraver instead of a Scheme engraver. This should help get it into the source tree before 2.14. Thanks, Carl
Sign in to reply to this message.
Hello Carl, wow, you were fast ... LGTM ;-) As said before, I wouldn't be able to do this engraver myself, but I think I understand now a tiny bit better how c++ engraver work. Thank you, Marc http://codereview.appspot.com/2723043/diff/1/lily/tab-tie-follow-engraver.cc File lily/tab-tie-follow-engraver.cc (right): http://codereview.appspot.com/2723043/diff/1/lily/tab-tie-follow-engraver.cc#... lily/tab-tie-follow-engraver.cc:34: are present. IMHO, the description should be more detailed, e.g. Change the tab-note-head properties when a tie is followed by a slur or a glissando. http://codereview.appspot.com/2723043/diff/1/lily/tab-tie-follow-engraver.cc#... lily/tab-tie-follow-engraver.cc:145: "Adjust tabNoteHead properties when accuring with ties," uppercase: TabNoteHead
Sign in to reply to this message.
I may be missing something, but doesn't this assume all line spanners are glissandi? Trevor http://codereview.appspot.com/2723043/diff/1/lily/tab-tie-follow-engraver.cc File lily/tab-tie-follow-engraver.cc (right): http://codereview.appspot.com/2723043/diff/1/lily/tab-tie-follow-engraver.cc#... lily/tab-tie-follow-engraver.cc:69: glissandi_.push_back (info.grob ()); Does this not catch line spanners other than glissandi??
Sign in to reply to this message.
On 2010/10/31 09:29:29, Trevor Daniels wrote: > I may be missing something, but doesn't this assume all line spanners are > glissandi? I thought the same thing. I haven't investigated it carefully; I was just translating Marc's Scheme engraver to C++. Any comments, Mark? Thanks, Carl
Sign in to reply to this message.
I've updated the comments and the doc string, and added a check for Glissando. Thanks, Carl
Sign in to reply to this message.
Hi Carl, This is too complicated (though that's really a criticism of Marc's Scheme engraver). The point of using 'tie-follow is that it defers the decision to parenthesize TabNoteHead to the point where it matters: in the callbacks for Glissando and Slur. Thus there should be no need to acknowledge glissandos and slurs in the engraver: we only need to check whether the tie's right bound is one of the acknowledged noteheads. Cheers, Neil http://codereview.appspot.com/2723043/diff/7001/lily/tab-tie-follow-engraver.cc File lily/tab-tie-follow-engraver.cc (right): http://codereview.appspot.com/2723043/diff/7001/lily/tab-tie-follow-engraver.... lily/tab-tie-follow-engraver.cc:69: if (info.grob ()->name () == "Glissando") If you needed to distinguish glissandos from other lines, it would be more idiomatic to add an interface (glissando-interface), then use info.grob->internal_has_interface (ly_symbol2scm ("glissando-interface")) http://codereview.appspot.com/2723043/diff/7001/lily/tab-tie-follow-engraver.... lily/tab-tie-follow-engraver.cc:98: scm_from_int (LEFT)); slurs_[j]->get_bound (LEFT) http://codereview.appspot.com/2723043/diff/7001/lily/tab-tie-follow-engraver.... lily/tab-tie-follow-engraver.cc:99: if (scm_call_1 (ly_lily_module_constant ("ly:grob?"), left_bound) == SCM_BOOL_T) I'm not sure this serves any useful purpose; unless there's something seriously wrong, all slur bounds are grobs (of class Item).
Sign in to reply to this message.
On 2010/10/31 22:24:17, Neil Puttock wrote: > Hi Carl, > > This is too complicated (though that's really a criticism of Marc's Scheme > engraver). > > The point of using 'tie-follow is that it defers the decision to parenthesize > TabNoteHead to the point where it matters: in the callbacks for Glissando and > Slur. Thus there should be no need to acknowledge glissandos and slurs in the > engraver: we only need to check whether the tie's right bound is one of the > acknowledged noteheads. > Ah, yes. I hadn't thought through the logic carefully; I was just implementing Marc's logic in C++. Glissandos and slurs are irrelevant for determining tied-to. A note is tied-to if it's at the right hand end of a tie. Whether it's parenthesized or not is determined by the presence of a split tie, a glissando, or a slur. So we don't need to acknowledge glissando or slur. Just tie and note_head. Got it! Thanks, Carl
Sign in to reply to this message.
On 2010/10/31 22:24:17, Neil Puttock wrote: > http://codereview.appspot.com/2723043/diff/7001/lily/tab-tie-follow-engraver.... > lily/tab-tie-follow-engraver.cc:69: if (info.grob ()->name () == "Glissando") > If you needed to distinguish glissandos from other lines, it would be more > idiomatic to add an interface (glissando-interface), then use > > info.grob->internal_has_interface (ly_symbol2scm ("glissando-interface")) > This is going away, so it won't apply to this patch (because we don't need to acknowledge glissandos). But if we did, and we added a glissando-interface, then instead of Tab_tie_follow_engraver::acknowledge_line_spanner wouldn't we just use Tab_tie_follow_engraver::acknowledge_glissando? But it would seem strange to me to add an interface with no properties that can be set, which is what I think I'd be doing if I added a glissando_interface. Any comment on this thought? Thanks, Carl
Sign in to reply to this message.
On 2010/10/31 22:40:57, Carl wrote: > This is going away, so it won't apply to this patch (because we don't need to > acknowledge glissandos). But if we did, and we added a glissando-interface, > then instead of Tab_tie_follow_engraver::acknowledge_line_spanner wouldn't we > just use > Tab_tie_follow_engraver::acknowledge_glissando? Yes. > But it would seem strange to me to add an interface with no properties that can > be set, > which is what I think I'd be doing if I added a glissando_interface. Any > comment on this thought? If you look in define-grob-interfaces.scm, you'll see several interfaces with no properties: most of them exist just to allow engravers to distinguish between grobs which would normally be lumped together (e.g., acknowledging line-spanner-interface, when you really want something more specific). Cheers, Neil
Sign in to reply to this message.
New patch set with simplified engraver -- it only acknowledges ties and note-heads, and it still works. Thanks, Neil! Carl
Sign in to reply to this message.
Further thought about this causes me to think the engraver should just be Tie_follow_engraver It doesn't change a TabNoteHead property, it changes a NoteHead property. The only reason it applies to TabNoteHeads is because it is in a TabVoice context. Am I wrong in thinking this? Thanks, Carl
Sign in to reply to this message.
Or perhaps the engraver should only listen to tab-note-heads, instead of to note-heads, since the tie-follow property is part of the tab-note-head interface. Thanks, Carl
Sign in to reply to this message.
Am 31.10.2010 23:24, schrieb n.puttock@gmail.com: > Hi Carl, > > This is too complicated (though that's really a criticism of Marc's > Scheme engraver). Thank you, Neil, for pointing this out! I was referring to your engraver example which checked for slurs, too, and I overlooked the fact that the slur callback just does the right thing when tie-follow is set. Sorry for causing so much trouble. Regards, Marc
Sign in to reply to this message.
On 11/1/10 1:53 AM, "Marc Hohl" <marc@hohlart.de> wrote: > > Sorry for causing so much trouble. > No trouble, Marc. You never need to apologize for working to improve LilyPond! Thanks, Carl
Sign in to reply to this message.
Updated to only acknowledge tab-note-head, not note-head. Thanks, Carl
Sign in to reply to this message.
Am 02.11.2010 04:04, schrieb Carl.D.Sorensen@gmail.com: > Updated to only acknowledge tab-note-head, not note-head. Makes perfect sense to me. Thanks for your work! Marc > > Thanks, > > Carl > > > http://codereview.appspot.com/2723043/ >
Sign in to reply to this message.
On 11/3/10 6:50 AM, "Marc Hohl" <marc@hohlart.de> wrote: > Am 02.11.2010 04:04, schrieb Carl.D.Sorensen@gmail.com: >> Updated to only acknowledge tab-note-head, not note-head. > Makes perfect sense to me. > Thanks for your work! You're welcome. Pushed to git. Thanks, Carl
Sign in to reply to this message.
Am 03.11.2010 15:10, schrieb Carl Sorensen: > > > On 11/3/10 6:50 AM, "Marc Hohl"<marc@hohlart.de> wrote: > > >> Am 02.11.2010 04:04, schrieb Carl.D.Sorensen@gmail.com: >> >>> Updated to only acknowledge tab-note-head, not note-head. >>> >> Makes perfect sense to me. >> Thanks for your work! >> > You're welcome. > > Pushed to git. > Oops, I found an error! While my engraver added 'tie-follow only to the notes followed by a slur or a glissando, your engraver adds this to *every* tied note, so the callbacks in tablature.scm are not working properly - *every* note that is 'tied to' gets parenthesized! Moreover, Neil's argument that slurs and glissandos do not belong to the engraver seems not to be valid - every note that is 'tied to' now gets the 'tie-follow mark, so the tie handler will make them invisible and the slur routine that may follow changes the visibility again, and that's exactly what Neil critiziced at my first approach (and he was right about that). So, IIUC, you'll have to use the version of your engraver with the slur and glissando stuff included. Sorry for understanding this so late, Marc > Thanks, > > Carl > > >
Sign in to reply to this message.
On 3 November 2010 16:15, Marc Hohl <marc@hohlart.de> wrote: > Am 03.11.2010 15:10, schrieb Carl Sorensen: >> >> >> On 11/3/10 6:50 AM, "Marc Hohl"<marc@hohlart.de> wrote: >> >> >>> >>> Am 02.11.2010 04:04, schrieb Carl.D.Sorensen@gmail.com: >>> >>>> >>>> Updated to only acknowledge tab-note-head, not note-head. >>>> >>> >>> Makes perfect sense to me. >>> Thanks for your work! >>> >> >> You're welcome. >> >> Pushed to git. >> > > Oops, I found an error! > > While my engraver added 'tie-follow only to the notes followed by a slur or > a glissando, > your engraver adds this to *every* tied note, so the callbacks in > tablature.scm are > not working properly - *every* note that is 'tied to' gets parenthesized! It's a shame Carl's already pushed the patch; I don't think it's ready yet. The code for checking bounds is wrong: as I noted for the original patchset, get_bound () is the correct method, rather than jumping into scheme (ly:spanner-bound). > Moreover, Neil's argument that slurs and glissandos do not belong to the > engraver > seems not to be valid - every note that is 'tied to' now gets the > 'tie-follow mark, so the > tie handler will make them invisible and the slur routine that may follow > changes the visibility > again, and that's exactly what Neil critiziced at my first approach (and he > was right about that). That's because the tie callback is interfering with the slur and glissando callbacks: you need to stop it making the noteheads transparent. Cheers, Neil
Sign in to reply to this message.
On 11/3/10 12:43 PM, "Neil Puttock" <n.puttock@gmail.com> wrote: > On 3 November 2010 16:15, Marc Hohl <marc@hohlart.de> wrote: >> Am 03.11.2010 15:10, schrieb Carl Sorensen: >>> >>> >>> On 11/3/10 6:50 AM, "Marc Hohl"<marc@hohlart.de> wrote: >>> >>> >>>> >>>> Am 02.11.2010 04:04, schrieb Carl.D.Sorensen@gmail.com: >>>> >>>>> >>>>> Updated to only acknowledge tab-note-head, not note-head. >>>>> >>>> >>>> Makes perfect sense to me. >>>> Thanks for your work! >>>> >>> >>> You're welcome. >>> >>> Pushed to git. >>> >> >> Oops, I found an error! >> >> While my engraver added 'tie-follow only to the notes followed by a slur or >> a glissando, >> your engraver adds this to *every* tied note, so the callbacks in >> tablature.scm are >> not working properly - *every* note that is 'tied to' gets parenthesized! > > It's a shame Carl's already pushed the patch; I don't think it's ready yet. I'm sorry. I'll revert it. > > The code for checking bounds is wrong: as I noted for the original > patchset, get_bound () is the correct method, rather than jumping into > scheme (ly:spanner-bound). Fixed. > >> Moreover, Neil's argument that slurs and glissandos do not belong to the >> engraver >> seems not to be valid - every note that is 'tied to' now gets the >> 'tie-follow mark, so the >> tie handler will make them invisible and the slur routine that may follow >> changes the visibility >> again, and that's exactly what Neil critiziced at my first approach (and he >> was right about that). > > That's because the tie callback is interfering with the slur and > glissando callbacks: you need to stop it making the noteheads > transparent. > But the tie callback *should* make the notehead transparent if there's no slur or gliss (or bend, in the future). In the absence of slur, gliss, or split tie the notehead is transparent. In the presence of one of these, it's visible and parenthesized. So how would one achieve this behavior without the tie callback making the notehead transparent? Thanks, Carl > Cheers, > Neil
Sign in to reply to this message.
On 11/3/10 10:15 AM, "Marc Hohl" <marc@hohlart.de> wrote: > Am 03.11.2010 15:10, schrieb Carl Sorensen: >> >> >> On 11/3/10 6:50 AM, "Marc Hohl"<marc@hohlart.de> wrote: >> >> >>> Am 02.11.2010 04:04, schrieb Carl.D.Sorensen@gmail.com: >>> >>>> Updated to only acknowledge tab-note-head, not note-head. >>>> >>> Makes perfect sense to me. >>> Thanks for your work! >>> >> You're welcome. >> >> Pushed to git. >> > Oops, I found an error! > > While my engraver added 'tie-follow only to the notes followed by a slur > or a glissando, > your engraver adds this to *every* tied note, so the callbacks in > tablature.scm are > not working properly - *every* note that is 'tied to' gets parenthesized! > > Moreover, Neil's argument that slurs and glissandos do not belong to the > engraver > seems not to be valid - every note that is 'tied to' now gets the > 'tie-follow mark, so the > tie handler will make them invisible and the slur routine that may > follow changes the visibility > again, and that's exactly what Neil critiziced at my first approach (and > he was right about that). So perhaps what we need are 'tie-follow, 'slur-start, and 'gliss-start properties for a tab note head. Or perhaps we need to change the name of the property from tie-follow to show-hidden. And the show-hidden property gets set only if it's tie-follow and slur-start, or tie-follow and gliss-start, or tie-follow and bend-start (in the future). Under this proposal, we'd change the engraver name to something like tab_tied_head_engraver. It wouldn't take too much to do this, but is it the *right* approach to solve this problem? Thanks, Carl
Sign in to reply to this message.
On 3 November 2010 19:33, Carl Sorensen <c_sorensen@byu.edu> wrote: > But the tie callback *should* make the notehead transparent if there's no > slur or gliss (or bend, in the future). In the absence of slur, gliss, or > split tie the notehead is transparent. In the presence of one of these, > it's visible and parenthesized. > > So how would one achieve this behavior without the tie callback making the > notehead transparent? The slur and Glissando callbacks should make the notehead visible again (this isn't ideal, but I can't see any other way of doing it without making the code more complex). Cheers, Neil
Sign in to reply to this message.
Hi Carl, What do you think about folding this code into the Tab_note_heads_engraver? We could store the created TabNoteHead grobs and add an acknowledger for Tie, then set 'tie-follow in stop_translation_timestep (). Cheers, Neil http://codereview.appspot.com/2723043/diff/31001/lily/tab-tie-follow-engraver.cc File lily/tab-tie-follow-engraver.cc (right): http://codereview.appspot.com/2723043/diff/31001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:72: SCM proc = ly_lily_module_constant ("ly:spanner-bound"); remove http://codereview.appspot.com/2723043/diff/31001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:75: SCM right_bound = ties_[1]->get_bound (LEFT); Item *right_bound = ties_[i]->get_bound (RIGHT); (will require casting or declaring ties_ as vector<Spanner *>) http://codereview.appspot.com/2723043/diff/31001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:78: if (right_bound == note_heads_[k]->self_scm ()) if (right_bound = note_heads_[k]) (again, casting required, or declare note_heads_ as vector<Item *>) http://codereview.appspot.com/2723043/diff/31001/scm/tablature.scm File scm/tablature.scm (right): http://codereview.appspot.com/2723043/diff/31001/scm/tablature.scm#newcode187 scm/tablature.scm:187: (if tie-follow the tie callback shouldn't care about 'tie-follow, so this extra code should be removed http://codereview.appspot.com/2723043/diff/31001/scm/tablature.scm#newcode223 scm/tablature.scm:223: (if tie-follow see comment for tie::handle-tab-note-head http://codereview.appspot.com/2723043/diff/31001/scm/tablature.scm#newcode266 scm/tablature.scm:266: (ly:grob-set-property! left-tab-note-head 'stencil add (set! (ly:grob-property tied-tab-note-head 'transparent) #t) http://codereview.appspot.com/2723043/diff/31001/scm/tablature.scm#newcode279 scm/tablature.scm:279: (ly:grob-set-property! left-tab-note-head 'stencil add (set! (ly:grob-property tied-tab-note-head 'transparent) #t)
Sign in to reply to this message.
On 2010/11/03 20:46:27, Neil Puttock wrote: > Hi Carl, > > What do you think about folding this code into the Tab_note_heads_engraver? We > could store the created TabNoteHead grobs and add an acknowledger for Tie, then > set 'tie-follow in stop_translation_timestep (). > I think I'd probably go a bit farther. What if we acknowledged ties, slurs, and glissandos (glissandi?) in the Tab_note_head_engraver and set the desired properties there. And then we eliminated all of the notehead handling from the slur callback, the tie callback, and the glissando callback. That way the notehead callback could take care of the notehead, and we wouldn't need to mix things at all. What do you think? Carl
Sign in to reply to this message.
On 2010/11/03 23:14:47, Carl wrote: > I think I'd probably go a bit farther. > > What if we acknowledged ties, slurs, and glissandos (glissandi?) in the > Tab_note_head_engraver and set the desired properties there. Which properties? If it involves multiple nested loops (like the original patch), then I'd say no. How would you deal with TabNoteHeads after a line break? IIRC, there is some code in tablature.scm which treats them differently, but this is something you can't do in an engraver. Cheers, Neil
Sign in to reply to this message.
On 11/3/10 5:52 PM, "n.puttock@gmail.com" <n.puttock@gmail.com> wrote: > On 2010/11/03 23:14:47, Carl wrote: > >> I think I'd probably go a bit farther. > >> What if we acknowledged ties, slurs, and glissandos (glissandi?) in > the >> Tab_note_head_engraver and set the desired properties there. > > Which properties? Well, it would seem that in order to know how to display a tab note head properly we need to know the following: 1. Is it on the right hand end of a tie ('tie-follow property) 2. Is it on the left hand end of a slur, glissando, or bend ('begin-spanner property?) > > If it involves multiple nested loops (like the original patch), then I'd > say no. I think we could avoid multiple nested loops, but there would be loops: First of all, instead of having slurs_, and glissandos_, we'd have spanners_, and all of the spanner grobs (slur, glissando, bend) would be pushed to the same vector In pseudocode For each tab-note-head for each tie if right-bound of tie is tab-note-head set 'tie-follow break end if end for for each spanner if left-bound of spanner is tab-note-head set 'begin-spanner break end if end for end for > > How would you deal with TabNoteHeads after a line break? IIRC, there is > some code in tablature.scm which treats them differently, but this is > something you can't do in an engraver. Right -- that would be part of the TabNoteHead callback -- it would look at the current status of the notehead to see if it's after a break, and make the appropriate decisions based on the 'tie-follow and 'begin-spanner properties. Thanks, Carl
Sign in to reply to this message.
Am 03.11.2010 21:22, schrieb Neil Puttock: > On 3 November 2010 19:33, Carl Sorensen<c_sorensen@byu.edu> wrote: > > >> But the tie callback *should* make the notehead transparent if there's no >> slur or gliss (or bend, in the future). In the absence of slur, gliss, or >> split tie the notehead is transparent. In the presence of one of these, >> it's visible and parenthesized. >> >> So how would one achieve this behavior without the tie callback making the >> notehead transparent? >> > The slur and Glissando callbacks should make the notehead visible > again (this isn't ideal, but I can't see any other way of doing it > without making the code more complex). > Why can't we use the mechanism we already had instead? The engraver sets tie-follow *only* for tab-note-heads which are right bounds of a tie *and* left bounds of either a slur or a glissando - then the callback code works properly, as far as I can see. Marc > Cheers, > Neil > >
Sign in to reply to this message.
Am 03.11.2010 20:33, schrieb Carl Sorensen: > > > On 11/3/10 10:15 AM, "Marc Hohl"<marc@hohlart.de> wrote: > > >> Am 03.11.2010 15:10, schrieb Carl Sorensen: >> >>> >>> On 11/3/10 6:50 AM, "Marc Hohl"<marc@hohlart.de> wrote: >>> >>> >>> >>>> Am 02.11.2010 04:04, schrieb Carl.D.Sorensen@gmail.com: >>>> >>>> >>>>> Updated to only acknowledge tab-note-head, not note-head. >>>>> >>>>> >>>> Makes perfect sense to me. >>>> Thanks for your work! >>>> >>>> >>> You're welcome. >>> >>> Pushed to git. >>> >>> >> Oops, I found an error! >> >> While my engraver added 'tie-follow only to the notes followed by a slur >> or a glissando, >> your engraver adds this to *every* tied note, so the callbacks in >> tablature.scm are >> not working properly - *every* note that is 'tied to' gets parenthesized! >> >> Moreover, Neil's argument that slurs and glissandos do not belong to the >> engraver >> seems not to be valid - every note that is 'tied to' now gets the >> 'tie-follow mark, so the >> tie handler will make them invisible and the slur routine that may >> follow changes the visibility >> again, and that's exactly what Neil critiziced at my first approach (and >> he was right about that). >> > So perhaps what we need are 'tie-follow, 'slur-start, and 'gliss-start > properties for a tab note head. > No, I don't think we should it do more complicated that necessary. Perhaps the name 'tie-follow is misleading, but the engraver (before you left out the slur and glissando bits) did the right job - marking exactly the tab-note-heads that have to be treated specially. If we mark *every* tied-to note, we have to mark *every* start of a slur and *every* start of a glissando as well and check for the appearance of (and ('tie-follow (or ( 'slur-start 'gliss-start)))), which is overkill - just let the engraver take the decision, raise a flag, and the callbacks do their job. Marc
Sign in to reply to this message.
On 2010/11/04 08:31:30, marc wrote:
> No, I don't think we should it do more complicated that necessary.
> Perhaps the name 'tie-follow is misleading, but the engraver (before you
> left out the slur and glissando bits) did the right job - marking exactly
> the tab-note-heads that have to be treated specially.
>
> If we mark *every* tied-to note, we have to mark *every* start of a slur
> and *every* start
> of a glissando as well and check for the appearance of (and ('tie-follow
> (or ( 'slur-start 'gliss-start)))),
> which is overkill - just let the engraver take the decision, raise a
> flag, and the callbacks do their job.
But right now, the callbacks are fighting over the notes -- and I don't think
that's right. In order to work correctly, we need to know the order in which
the callbacks are called.
I've got an algorithm that I think is clearer and simplifies the callbacks, but
I haven't been able to fully test it yet because I can't get the C++ engraver to
work right in terms of checking equality.
I'll post a patch for comments.
Thanks,
Carl
Sign in to reply to this message.
This patch has revised the callbacks in scm/tablature.scm. It appears to work properly. The tab-tie-follow-engraver is still using SCM calls to analyze the grobs. I will get that worked out soon, I hope (although I seem to have gone backward, not forward, today). In this patch, the glissando, slur, tie, and repeat-tie callbacks have been modified. The slur callback doesn't even check for the tie status, since there's no change. The glissando callback checks to see if the notehead has been forced to display with parentheses, but it does not set the notehead display format. The tie and repeat-tie callbacks are responsible for setting the notehead display. I think that this is the right logic. Because the tie can be split, the tie (and repeat-tie) callbacks need to decide on the note head display. The slur and glissando callbacks don't need to set the display, they just respond to it if need be. Thanks, Carl
Sign in to reply to this message.
Hello Carl, I didn't test the patch, but the general logic behind it seems to be the right way to do. Regards, Marc http://codereview.appspot.com/2723043/diff/57001/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): http://codereview.appspot.com/2723043/diff/57001/scm/define-grob-properties.s... scm/define-grob-properties.scm:1021: (display-cautionary ,boolean? "Display in cautionary form.") Hey, good choice - I like the name of the new property, it's much better than tie-follow. http://codereview.appspot.com/2723043/diff/57001/scm/tablature.scm File scm/tablature.scm (right): http://codereview.appspot.com/2723043/diff/57001/scm/tablature.scm#newcode267 scm/tablature.scm:267: ;; (and tie-follow this should be 'cautionary', IIUC.
Sign in to reply to this message.
Am 07.11.2010 08:47, schrieb Carl.D.Sorensen@gmail.com: > This patch has revised the callbacks in scm/tablature.scm. It appears > to work properly. > > The tab-tie-follow-engraver is still using SCM calls to analyze the > grobs. I will get that worked out soon, I hope (although I seem to have > gone backward, not forward, today). > > In this patch, the glissando, slur, tie, and repeat-tie callbacks have > been modified. The slur callback doesn't even check for the tie status, > since there's no change. But it has to take the value of 'display-cautionary into account, isn't it? > The glissando callback checks to see if the > notehead has been forced to display with parentheses, but it does not > set the notehead display format. The tie and repeat-tie callbacks are > responsible for setting the notehead display. > > I think that this is the right logic. Because the tie can be split, the > tie (and repeat-tie) callbacks need to decide on the note head display. > The slur and glissando callbacks don't need to set the display, they > just respond to it if need be. +1 Marc > > Thanks, > > Carl > > > http://codereview.appspot.com/2723043/ >
Sign in to reply to this message.
On 2010/11/07 09:21:45, marc wrote: > http://codereview.appspot.com/2723043/diff/57001/scm/define-grob-properties.s... > scm/define-grob-properties.scm:1021: (display-cautionary ,boolean? "Display in > cautionary form.") > Hey, good choice - I like the name of the new property, > it's much better than tie-follow. Thanks. I hoped you wouldn't mind. > http://codereview.appspot.com/2723043/diff/57001/scm/tablature.scm#newcode267 > scm/tablature.scm:267: ;; (and tie-follow > this should be 'cautionary', IIUC. This is commented out and will go away once I'm sure everything is working properly.
Sign in to reply to this message.
On 2010/11/07 09:24:31, marc wrote: > Am 07.11.2010 08:47, schrieb Carl.D.Sorensen@gmail.com: > > In this patch, the glissando, slur, tie, and repeat-tie callbacks have > > been modified. The slur callback doesn't even check for the tie status, > > since there's no change. > But it has to take the value of 'display-cautionary into account, isn't it? As far as I could see from the existing code, there was no difference for the slur based on the style of the note. The slur goes from the top center of the note, which is the same in both cases, I think. > > I think that this is the right logic. Because the tie can be split, the > > tie (and repeat-tie) callbacks need to decide on the note head display. > > The slur and glissando callbacks don't need to set the display, they > > just respond to it if need be. > +1 Thanks for the confirmation. Carl
Sign in to reply to this message.
Here's a new version of the patch that tries to eliminate any scheme calls, and demonstrates my problem. I need to do a check on the left bound to see if it's a grob, because if I take the check out I get a Bus error. I don't know how to do such a check from C++ -- I think it's done by casting, but I'm not sure. If I self-scm() the C++ Item * variable, then use the scheme routine ly:grob? to check if it's a grob, if I get a Bus error. Neil, can you give me any insight here? I'd like to use all C++ variables, and eliminate the two remaining scheme calls. As it stands, the code works on my machine. Thanks, Carl
Sign in to reply to this message.
Am 12.11.2010 05:07, schrieb Carl.D.Sorensen@gmail.com: > On 2010/11/07 09:21:45, marc wrote: > > http://codereview.appspot.com/2723043/diff/57001/scm/define-grob-properties.s... > >> scm/define-grob-properties.scm:1021: (display-cautionary ,boolean? > "Display in >> cautionary form.") >> Hey, good choice - I like the name of the new property, >> it's much better than tie-follow. > > Thanks. I hoped you wouldn't mind. > > > > http://codereview.appspot.com/2723043/diff/57001/scm/tablature.scm#newcode267 > >> scm/tablature.scm:267: ;; (and tie-follow >> this should be 'cautionary', IIUC. > > This is commented out and will go away once I'm sure everything is > working properly. Ah, I overlooked the ;; - and of course, since the engraver has done its work before the callbacks take place, there is no need for the slur callback to check the 'cautionary property, and the same goes for the glissandos (glissandi? whatever...) Now IUC ;-) Thanks for your work, Marc > > > > http://codereview.appspot.com/2723043/ >
Sign in to reply to this message.
On 2010/11/12 04:12:48, Carl wrote: > Here's a new version of the patch that tries to eliminate any scheme calls, and > demonstrates my problem. > > I need to do a check on the left bound to see if it's a grob, because if I take > the check out I get a Bus error. > > I don't know how to do such a check from C++ -- I think it's done by casting, > but I'm not sure. Of course you know. It's a pointer, so you check whether it's NULL. if (left_bound) > If I self-scm() the C++ Item * variable, then use the scheme routine ly:grob? to > check if it's a grob, if I get a Bus error. The slur appears to be acknowledged twice, and the first time it has no left bound (which implies it would be more efficient and safer to do this checking in stop_translation_timestep ()) > As it stands, the code works on my machine. It's back to square one though, isn't it? The triple nested loop is horrible, and should be avoided at all costs; this is partly why I suggested only acknowledging the ties and heads. Cheers, Neil
Sign in to reply to this message.
Thanks for the help on the null pointer. I was thinking that it was some *other* kind of variable than a Grob, and that I was casting it wrong. Got that all taken care of -- no scheme calls at all. On 2010/11/12 17:40:42, Neil Puttock wrote: > > It's back to square one though, isn't it? > > The triple nested loop is horrible, and should be avoided at all costs; this is > partly why I suggested only acknowledging the ties and heads. OK, so now I've eliminated the triple nested loop. There is what appears to me to be a required nested loop. One loop to loop through the note-heads. Then an inner loop (with a break) through the ties looking for a tie on a note head. Followed by a second inner loop (with a break) through the slurs looking for a slur on the note head. Followed by a third inner loop (if we didn't find a slur) through the glissandos looking for a glissando on the note head. I can think of no way to simplify this code further. If you have any ideas I'd be happy to hear them. Thanks, Carl
Sign in to reply to this message.
Am 13.11.2010 06:21, schrieb Carl.D.Sorensen@gmail.com: > Thanks for the help on the null pointer. I was thinking that it was > some *other* kind of variable than a Grob, and that I was casting it > wrong. > > Got that all taken care of -- no scheme calls at all. > > On 2010/11/12 17:40:42, Neil Puttock wrote: > >> It's back to square one though, isn't it? > >> The triple nested loop is horrible, and should be avoided at all > costs; this is >> partly why I suggested only acknowledging the ties and heads. > > OK, so now I've eliminated the triple nested loop. > > There is what appears to me to be a required nested loop. > > One loop to loop through the note-heads. > Then an inner loop (with a break) through the ties looking for a tie on > a note head. > Followed by a second inner loop (with a break) through the slurs looking > for a slur on the note head. > Followed by a third inner loop (if we didn't find a slur) through the > glissandos looking for a glissando on the note head. > > I can think of no way to simplify this code further. If you have any > ideas I'd be happy to hear them. There was the idea to include this into the Tab_note_heads_engraver, and if it were possible to include the parenthesize stuff into that engraver as well, it would a) simplify the callbacks b) solve the problem about the harmonic brackets Is it possible to detect the tie/slur/glissando cases within the engraver and call a routine to parenthesize the fret number? Then the resulting grob is a compound object that gets its angle brackets displayed correctly without fiddling with padding and stuff. Marc > > Thanks, > > Carl > > > http://codereview.appspot.com/2723043/ >
Sign in to reply to this message.
On 11/13/10 3:18 AM, "Marc Hohl" <marc@hohlart.de> wrote: > Am 13.11.2010 06:21, schrieb Carl.D.Sorensen@gmail.com: >> I can think of no way to simplify this code further. If you have any >> ideas I'd be happy to hear them. > There was the idea to include this into the Tab_note_heads_engraver, and > if it were > possible to include the parenthesize stuff into that engraver as well, > it would > > a) simplify the callbacks > b) solve the problem about the harmonic brackets > > Is it possible to detect the tie/slur/glissando cases within the > engraver and call a > routine to parenthesize the fret number? Then the resulting grob is a > compound object that > gets its angle brackets displayed correctly without fiddling with > padding and stuff. It may be possible to include the tie/slur/glissando in the tab_note_heads_engraver, but I haven't yet been able to get it to work properly. However, we can't do the whole display-cautionary thing in the engraver, because the engraver doesn't know about such things as split ties. That has to be dealt with in the tab note head callback. The problem with the harmonic brackets is orthogonal to this problem. I started work on it, but I have to sort out the various parenthesize functions before I know how to fully deal with it. It's on my list, but I want to get this one resolved first. Thanks, Carl
Sign in to reply to this message.
Am 13.11.2010 15:29, schrieb Carl Sorensen: > On 11/13/10 3:18 AM, "Marc Hohl"<marc@hohlart.de> wrote: > > >> Am 13.11.2010 06:21, schrieb Carl.D.Sorensen@gmail.com: >> > >>> I can think of no way to simplify this code further. If you have any >>> ideas I'd be happy to hear them. >>> >> There was the idea to include this into the Tab_note_heads_engraver, and >> if it were >> possible to include the parenthesize stuff into that engraver as well, >> it would >> >> a) simplify the callbacks >> b) solve the problem about the harmonic brackets >> >> Is it possible to detect the tie/slur/glissando cases within the >> engraver and call a >> routine to parenthesize the fret number? Then the resulting grob is a >> compound object that >> gets its angle brackets displayed correctly without fiddling with >> padding and stuff. >> > It may be possible to include the tie/slur/glissando in the > tab_note_heads_engraver, but I haven't yet been able to get it to work > properly. > > However, we can't do the whole display-cautionary thing in the engraver, > because the engraver doesn't know about such things as split ties. That has > to be dealt with in the tab note head callback. > Yes, I know, because the line breaking happens at a time when the engravers have already done their job, IIUC. > The problem with the harmonic brackets is orthogonal to this problem. I > started work on it, but I have to sort out the various parenthesize > functions before I know how to fully deal with it. It's on my list, but I > want to get this one resolved first. > Yes, of course. I am not sure whether it is possible at all to include all the stuff into the tab note head engraver, but I think it would be more homogenic to handle the tab note head appearance by engravers as far as possible without callback patchwork. Marc > Thanks, > > Carl > > >
Sign in to reply to this message.
http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver.cc File lily/tab-tie-follow-engraver.cc (right): http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:52: void process_acknowledged (); remove http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:64: ties_.push_back (dynamic_cast <Spanner *> (info.grob ())); push_back (info.spanner ()) http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:70: glissandi_.push_back (dynamic_cast <Spanner *> (info.grob ())); push_back (info.spanner ()) http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:76: note_heads_.push_back (dynamic_cast<Item *> (info.grob ())); push_back (info.item ()) http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:82: slurs_.push_back (dynamic_cast<Spanner *> (info.grob ())); push_back (info.spanner ()) http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:86: Tab_tie_follow_engraver::process_acknowledged () remove http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:108: Item *slur_cause = dynamic_cast<Item *> (unsmob_grob (left_cause)); unsmob_item (left_cause) http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:121: if ((left_bound == note_heads_[k])) remove extra parentheses http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:154: gratuitous newline http://codereview.appspot.com/2723043/diff/70001/scm/define-grob-interfaces.scm File scm/define-grob-interfaces.scm (right): http://codereview.appspot.com/2723043/diff/70001/scm/define-grob-interfaces.s... scm/define-grob-interfaces.scm:214: '(details display-cautionary)) ? tie-follow span-start http://codereview.appspot.com/2723043/diff/70001/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): http://codereview.appspot.com/2723043/diff/70001/scm/define-grob-properties.s... scm/define-grob-properties.scm:1016: (span-start ,boolean? "Is the note at the start of a spanner?") note head http://codereview.appspot.com/2723043/diff/70001/scm/define-grob-properties.s... scm/define-grob-properties.scm:1023: (tie-follow ,boolean? "Is the note at the end of a tie?") note head http://codereview.appspot.com/2723043/diff/70001/scm/define-grobs.scm File scm/define-grobs.scm (right): http://codereview.appspot.com/2723043/diff/70001/scm/define-grobs.scm#newcode822 scm/define-grobs.scm:822: line-interface indent
Sign in to reply to this message.
Thanks for the review, Neil. I've responded to all your comments. I've also defined a new print function for TabNoteHeads in Scheme. It will take care of all of the necessary parentheses and harmonic brackets, based on the settings of 'display-cautionary and 'style. Thanks, Carl http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver.cc File lily/tab-tie-follow-engraver.cc (right): http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:52: void process_acknowledged (); On 2010/11/16 23:30:42, Neil Puttock wrote: > remove Done. http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:64: ties_.push_back (dynamic_cast <Spanner *> (info.grob ())); On 2010/11/16 23:30:42, Neil Puttock wrote: > push_back (info.spanner ()) Done. http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:70: glissandi_.push_back (dynamic_cast <Spanner *> (info.grob ())); On 2010/11/16 23:30:42, Neil Puttock wrote: > push_back (info.spanner ()) Done. Thanks for teaching me about these calls. http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:76: note_heads_.push_back (dynamic_cast<Item *> (info.grob ())); On 2010/11/16 23:30:42, Neil Puttock wrote: > push_back (info.item ()) Done. http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:82: slurs_.push_back (dynamic_cast<Spanner *> (info.grob ())); On 2010/11/16 23:30:42, Neil Puttock wrote: > push_back (info.spanner ()) Done. http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:86: Tab_tie_follow_engraver::process_acknowledged () On 2010/11/16 23:30:42, Neil Puttock wrote: > remove Done. http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:108: Item *slur_cause = dynamic_cast<Item *> (unsmob_grob (left_cause)); On 2010/11/16 23:30:42, Neil Puttock wrote: > unsmob_item (left_cause) Done. http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:121: if ((left_bound == note_heads_[k])) On 2010/11/16 23:30:42, Neil Puttock wrote: > remove extra parentheses Done. http://codereview.appspot.com/2723043/diff/70001/lily/tab-tie-follow-engraver... lily/tab-tie-follow-engraver.cc:154: On 2010/11/16 23:30:42, Neil Puttock wrote: > gratuitous newline Done. http://codereview.appspot.com/2723043/diff/70001/scm/define-grob-interfaces.scm File scm/define-grob-interfaces.scm (right): http://codereview.appspot.com/2723043/diff/70001/scm/define-grob-interfaces.s... scm/define-grob-interfaces.scm:214: '(details display-cautionary)) On 2010/11/16 23:30:42, Neil Puttock wrote: New structure is span-start and display-cautionary. tie-follow is implied in the callback that is used. display-cautionary is used by glissando callback and in new tab-note-head::print function. http://codereview.appspot.com/2723043/diff/70001/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): http://codereview.appspot.com/2723043/diff/70001/scm/define-grob-properties.s... scm/define-grob-properties.scm:1016: (span-start ,boolean? "Is the note at the start of a spanner?") On 2010/11/16 23:30:42, Neil Puttock wrote: > note head Done. http://codereview.appspot.com/2723043/diff/70001/scm/define-grob-properties.s... scm/define-grob-properties.scm:1023: (tie-follow ,boolean? "Is the note at the end of a tie?") On 2010/11/16 23:30:42, Neil Puttock wrote: > note head Eliminated http://codereview.appspot.com/2723043/diff/70001/scm/define-grobs.scm File scm/define-grobs.scm (right): http://codereview.appspot.com/2723043/diff/70001/scm/define-grobs.scm#newcode822 scm/define-grobs.scm:822: line-interface On 2010/11/16 23:30:42, Neil Puttock wrote: > indent I assume you mean to use tabs. Done.
Sign in to reply to this message.
Hello Carl, your code looks great (at least at a quick glance), but it looks as you didn't rebase after my patch correction concerning the custom fret label was applied. http://codereview.appspot.com/2723043/diff/103001/scm/tablature.scm File scm/tablature.scm (right): http://codereview.appspot.com/2723043/diff/103001/scm/tablature.scm#newcode291 scm/tablature.scm:291: (grob-interpret-markup grob (make-customFretLabel-markup fret))) I think you reverted my corrected patch here, at least partly. customFretLabel-markup isn't defined any more.
Sign in to reply to this message.
Thanks, Marc. That was a mistake I made in resolving merge conflicts. http://codereview.appspot.com/2723043/diff/103001/scm/tablature.scm File scm/tablature.scm (right): http://codereview.appspot.com/2723043/diff/103001/scm/tablature.scm#newcode291 scm/tablature.scm:291: (grob-interpret-markup grob (make-customFretLabel-markup fret))) On 2010/11/27 09:05:58, marc wrote: > I think you reverted my corrected patch here, at least partly. > customFretLabel-markup isn't defined any more. Oops, I'm sorry. I handled the merge conflict inappropriately. I've now changed it to redefine tab-note-head::print-custom-fret-label so that it sets the 'text property of the grob and then calls tab-note-head::print.
Sign in to reply to this message.
Just some remarks about the harmonic detection. Regards, Marc http://codereview.appspot.com/2723043/diff/109001/scm/tablature.scm File scm/tablature.scm (right): http://codereview.appspot.com/2723043/diff/109001/scm/tablature.scm#newcode296 scm/tablature.scm:296: (harmonic (eq? (ly:grob-property grob 'style #f) 'harmonic)) I am not sure whether this approach is sane enough; I would at least include mixed-harmonic, too. When I worked on this, I used a different function: (define (is-harmonic? grob) (let ((articulations (ly:event-property (event-cause grob) 'articulations)) (harmonic-found #f)) (for-each (lambda (art) (if (ly:in-event-class? art 'harmonic-event) (set! harmonic-found #t))) articulations) harmonic-found)) This seems to work regardless of the user's choice of harmonic display style.
Sign in to reply to this message.
http://codereview.appspot.com/2723043/diff/109001/lily/tab-harmonic-engraver.cc File lily/tab-harmonic-engraver.cc (left): http://codereview.appspot.com/2723043/diff/109001/lily/tab-harmonic-engraver.... lily/tab-harmonic-engraver.cc:83: "HarmonicParenthesesItem ", This is never created, even for \tabFullNotation. http://codereview.appspot.com/2723043/diff/109001/lily/tab-harmonic-engraver.cc File lily/tab-harmonic-engraver.cc (right): http://codereview.appspot.com/2723043/diff/109001/lily/tab-harmonic-engraver.... lily/tab-harmonic-engraver.cc:59: victim->set_property ("style", ly_symbol2scm ("harmonic")); This makes it difficult for users to tweak the appearance of the harmonic brackets, though if you still think it's a price worth paying, you might as well junk the engraver and read 'articulations directly inside the notehead callback. http://codereview.appspot.com/2723043/diff/109001/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): http://codereview.appspot.com/2723043/diff/109001/scm/define-grob-properties.... scm/define-grob-properties.scm:965: (display-cautionary ,boolean? "Display as cautionary.") Needs a more descriptive docstring. http://codereview.appspot.com/2723043/diff/109001/scm/tablature.scm File scm/tablature.scm (right): http://codereview.appspot.com/2723043/diff/109001/scm/tablature.scm#newcode297 scm/tablature.scm:297: (whiteout (ly:grob-property grob 'whiteout #t)) If 'whiteout is set, it's applied to the final stencil in grob.cc. http://codereview.appspot.com/2723043/diff/109001/scm/tablature.scm#newcode321 scm/tablature.scm:321: (centered-stencil output-grob))) This has a nasty side-effect: it centres the noteheads on the PaperColumn, shifting them to the left of heads in other staves.
Sign in to reply to this message.
On 2010/11/28 22:24:05, Neil Puttock wrote: > http://codereview.appspot.com/2723043/diff/109001/lily/tab-harmonic-engraver.... > lily/tab-harmonic-engraver.cc:59: victim->set_property ("style", ly_symbol2scm > ("harmonic")); > This makes it difficult for users to tweak the appearance of the harmonic > brackets, though if you still think it's a price worth paying, you might as well > junk the engraver and read 'articulations directly inside the notehead callback. > Ahh, now I understand why there's a HarmonicParenthesisItem. I'm not sure what I think the best solution is. I'd welcome some discussion on it. I think that the harmonic parentheses should become part of the notehead grob, so that when we parenthesize the notehead it will include the harmonic parentheses. I can see that we don't have any way to adjust the properties if it's the part of the same grob, so we re really would like it to be two separate grobs. Is there any way you can think of to combine two grobs into one? > http://codereview.appspot.com/2723043/diff/109001/scm/tablature.scm#newcode321 > scm/tablature.scm:321: (centered-stencil output-grob))) > This has a nasty side-effect: it centres the noteheads on the PaperColumn, > shifting them to the left of heads in other staves. So what I'd really like to do is center the tab-note-heads on the center of the notes in the regular music column. I can do that by centering, then offsetting. But how can I get the amount for the offset? Thanks, Carl
Sign in to reply to this message.
On 2010/11/28 22:24:05, Neil Puttock wrote: http://codereview.appspot.com/2723043/diff/109001/scm/tablature.scm#newcode321 > scm/tablature.scm:321: (centered-stencil output-grob))) > This has a nasty side-effect: it centres the noteheads on the PaperColumn, > shifting them to the left of heads in other staves. I've now added code that sets an offset the size of an "8" in the current tab-note-head grob context. This actually lines things up better than it used to be lined up, in my opinion. Is it too much of a hack?
Sign in to reply to this message.
On 2010/11/28 15:42:47, marc wrote: > Just some remarks about the harmonic detection. Thanks, Marc. I wrote a slightly different version of this (using filter). It works great!
Sign in to reply to this message.
Thanks for the feedback. I've responded to the comments and posted a new patch set. http://codereview.appspot.com/2723043/diff/109001/lily/tab-harmonic-engraver.cc File lily/tab-harmonic-engraver.cc (left): http://codereview.appspot.com/2723043/diff/109001/lily/tab-harmonic-engraver.... lily/tab-harmonic-engraver.cc:83: "HarmonicParenthesesItem ", On 2010/11/28 22:24:05, Neil Puttock wrote: > This is never created, even for \tabFullNotation. Deleting the engraver solved this issue http://codereview.appspot.com/2723043/diff/109001/lily/tab-harmonic-engraver.cc File lily/tab-harmonic-engraver.cc (right): http://codereview.appspot.com/2723043/diff/109001/lily/tab-harmonic-engraver.... lily/tab-harmonic-engraver.cc:59: victim->set_property ("style", ly_symbol2scm ("harmonic")); On 2010/11/28 22:24:05, Neil Puttock wrote: > This makes it difficult for users to tweak the appearance of the harmonic > brackets, though if you still think it's a price worth paying, you might as well > junk the engraver and read 'articulations directly inside the notehead callback. I junked the engraver and added properties to the tab-note-head 'details for cautionary and harmonic brackets. http://codereview.appspot.com/2723043/diff/109001/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): http://codereview.appspot.com/2723043/diff/109001/scm/define-grob-properties.... scm/define-grob-properties.scm:965: (display-cautionary ,boolean? "Display as cautionary.") On 2010/11/28 22:24:05, Neil Puttock wrote: > Needs a more descriptive docstring. Tried to make it more descriptive, but I didn't want it to be limited to tab-note-head. http://codereview.appspot.com/2723043/diff/109001/scm/tablature.scm File scm/tablature.scm (right): http://codereview.appspot.com/2723043/diff/109001/scm/tablature.scm#newcode297 scm/tablature.scm:297: (whiteout (ly:grob-property grob 'whiteout #t)) On 2010/11/28 22:24:05, Neil Puttock wrote: > If 'whiteout is set, it's applied to the final stencil in grob.cc. Removed it from the callback http://codereview.appspot.com/2723043/diff/109001/scm/tablature.scm#newcode321 scm/tablature.scm:321: (centered-stencil output-grob))) On 2010/11/28 22:24:05, Neil Puttock wrote: > This has a nasty side-effect: it centres the noteheads on the PaperColumn, > shifting them to the left of heads in other staves. Added an offset to get the column centered one character-width to the right of the paper column. I think it's even better than it used to be.
Sign in to reply to this message.
On 2010/12/03 16:32:07, Carl wrote: > Added an offset to get the column centered one character-width to the right of > the paper column. I think it's even better than it used to be. They look too far to the right to me.
Sign in to reply to this message.
On 12/4/10 4:50 PM, "n.puttock@gmail.com" <n.puttock@gmail.com> wrote: > On 2010/12/03 16:32:07, Carl wrote: > >> Added an offset to get the column centered one character-width to the > right of >> the paper column. I think it's even better than it used to be. > > They look too far to the right to me. How's this? To my eye it appears that the tab heads are centered on the regular notes now. Thanks, Carl
Sign in to reply to this message.
On 5 December 2010 00:35, Carl Sorensen <c_sorensen@byu.edu> wrote: > How's this? To my eye it appears that the tab heads are centered on the > regular notes now. I don't know, but they're consistently shifted to the right unless there are double-digit notes present. Cheers, Neil
Sign in to reply to this message.
On 2010/12/05 00:58:13, Neil Puttock wrote: > I don't know, but they're consistently shifted to the right unless > there are double-digit notes present. I wanted your opinion on the spacing in the .png I sent. I adjusted the offset, but it wasn't part of this patch. I have deliberately shifted the tab note heads to the right, because they were slightly too far to the left, in my opinion. I've now put the new offsets in the latest patch sets. Thanks, Carl
Sign in to reply to this message.
On 2010/12/05 00:58:13, Neil Puttock wrote: > I don't know, but they're consistently shifted to the right unless > there are double-digit notes present. I wanted your opinion on the spacing in the .png I sent. I adjusted the offset, but it wasn't part of this patch. I have deliberately shifted the tab note heads to the right, because they were slightly too far to the left, in my opinion. I've now put the new offsets in the latest patch sets. Thanks, Carl
Sign in to reply to this message.
Am 05.12.2010 02:19, schrieb Carl.D.Sorensen@gmail.com: > On 2010/12/05 00:58:13, Neil Puttock wrote: >> I don't know, but they're consistently shifted to the right unless >> there are double-digit notes present. > > I wanted your opinion on the spacing in the .png I sent. I adjusted the > offset, but it wasn't part of this patch. > > I have deliberately shifted the tab note heads to the right, because > they were slightly too far to the left, in my opinion. I don't know why, but in the .png it looks as if they were almost right-aligned to the note heads, but in fact, they are centered. Perhaps the left edge of a number should be aligned to the left edge of a note head when the fret number is a single digit, whereas fret numbers > 9 should be centered according to the center of the left-aligned single digits (I don't know how to describe this in a simpler way...). Regards, Marc
Sign in to reply to this message.
On 12/5/10 12:22 PM, "Marc Hohl" <marc@hohlart.de> wrote: > Am 05.12.2010 02:19, schrieb Carl.D.Sorensen@gmail.com: >> On 2010/12/05 00:58:13, Neil Puttock wrote: >>> I don't know, but they're consistently shifted to the right unless >>> there are double-digit notes present. >> >> I wanted your opinion on the spacing in the .png I sent. I adjusted the >> offset, but it wasn't part of this patch. >> >> I have deliberately shifted the tab note heads to the right, because >> they were slightly too far to the left, in my opinion. > I don't know why, but in the .png it looks as if they were almost > right-aligned to the > note heads, but in fact, they are centered. > > Perhaps the left edge of a number should be aligned to the left edge of > a note head > when the fret number is a single digit, whereas fret numbers > 9 should > be centered > according to the center of the left-aligned single digits (I don't know how > to describe this in a simpler way...). > Marc, If you get the most recent patch from Rietveld, and look at line 276 of scm/tablatures.scm, you'll see the constant that adjusts things -- the 2/3. Would you please test this with multiple values of that constant, and come up with a value that you like? I think a value of 1/2 would give the behavior you described above. I'd like your opinion on what would be a preferred value. Once you have a good value, I think I'll modify the patch so that constant is part of the TabNoteHead 'details, which will give the user control over it. Thanks, Carl
Sign in to reply to this message.
Am 05.12.2010 23:12, schrieb Carl Sorensen: > > > [...] >> Perhaps the left edge of a number should be aligned to the left edge of >> a note head >> when the fret number is a single digit, whereas fret numbers> 9 should >> be centered >> according to the center of the left-aligned single digits (I don't know how >> to describe this in a simpler way...). >> >> > Marc, > > If you get the most recent patch from Rietveld, and look at line 276 of > scm/tablatures.scm, you'll see the constant that adjusts things -- the 2/3. > Hello Carl, I fiddled around with the value, and it seems that 3/5 is the best IMHO. 1/2 is exactly what I described, but it doesn't look right. > Would you please test this with multiple values of that constant, and come > up with a value that you like? > > I think a value of 1/2 would give the behavior you described above. > > I'd like your opinion on what would be a preferred value. > > > Once you have a good value, I think I'll modify the patch so that constant > is part of the TabNoteHead 'details, which will give the user control over > it. > > Sounds good! So anyone who feels unhappy with 3/5 could change this easily ;-) Regards, Marc
Sign in to reply to this message.
On 12/6/10 3:37 AM, "Marc Hohl" <marc@hohlart.de> wrote: > Am 05.12.2010 23:12, schrieb Carl Sorensen: >> >> >> [...] >>> Perhaps the left edge of a number should be aligned to the left edge of >>> a note head >>> when the fret number is a single digit, whereas fret numbers> 9 should >>> be centered >>> according to the center of the left-aligned single digits (I don't know how >>> to describe this in a simpler way...). >>> >>> >> Marc, >> >> If you get the most recent patch from Rietveld, and look at line 276 of >> scm/tablatures.scm, you'll see the constant that adjusts things -- the 2/3. >> > Hello Carl, > > I fiddled around with the value, and it seems that 3/5 is the best IMHO. > 1/2 is exactly what I described, but it doesn't look right. I didn't like 1/2 either. That's why I went with 2/3, but that was just because it seemed to be centered on the intersection between the regular note heads and the staff lines for notes in staff spaces. I'm happy to go with 3/5, as you recommend. I've added the head-offset property to TabNoteHead 'details, set its default value to 3/5, and pushed the changes. Thanks, Carl
Sign in to reply to this message.
Am 07.12.2010 00:56, schrieb Carl Sorensen: > [...] > I'm happy to go with 3/5, as you recommend. > > I've added the head-offset property to TabNoteHead 'details, set its default > value to 3/5, and pushed the changes. > Thanks! It's great to see how older lilypond input files are now compiled and the resulting scores look much better! Regards, Marc
Sign in to reply to this message.
|
