|
|
DescriptionFixes NoteColumn vs SpanBar collisions.
Patch Set 1 #Patch Set 2 : Fixes texidoc in span-bar-spacing.ly #Patch Set 3 : Assures correct horizontal space for prefatory and loose columns. #Patch Set 4 : Brings TimeSignature extra-spacing-height to span StaffSymbol #
Total comments: 3
Patch Set 5 : Incorporates Keith's suggestions. #Patch Set 6 : Incorporates Keith's suggestions. #
Total comments: 2
Patch Set 7 : Fixes grace note spacing problem. #
Total comments: 6
Patch Set 8 : Makes function in the pure-from-neighbor-interface easier to read. #Patch Set 9 : Allows grobs to be selective about their neighbors. #Patch Set 10 : Close to pure nirvana. #
Total comments: 2
Patch Set 11 : Gets rid of useless property. #Patch Set 12 : Fixes variable naming and a function in the pure-from-neighbor interface. #
Total comments: 10
Patch Set 13 : Incorporates more useful comments, deletes misleading ones. #Patch Set 14 : Uber-patch that can be pushed as separate commits. #Patch Set 15 : Adds pure-from-neighbor-interface to key signatures #Patch Set 16 : Compiles cleanly on master. #Patch Set 17 : First step - fixing span bar collisions. #
Total comments: 2
Patch Set 18 : Incorporates Keith's suggestonis. #
MessagesTotal messages: 50
Hey all, This fixes the SpanBar regression ID'd by Phil. It also fixes a few other things: 1) The research I sent out about accidentals not hanging over barlines (check my e-mails to the list from Oct. 6, Subject: First stab at getting bar extents right) is actualized here. 2) The elimination of skyline-vertical-padding corrects a horizontal spacing issue where the cue-clef takes up too much space in cue-clef-after-barline.ly and beam-collision-feasible-region.ly 3) A new regtest, lyrics-pass-under-bar.ly, shows a bug fix for lyrics. The patch passes regtests (I have yet to do a full doc build). The only concern I have is that a lot of regtests that combine tablature and normal notation are effected but I cannot see how (lots of green, but nothing looks like it moved). I don't know why my code is doing this, and I'm always hesitant to push code that has an effect that I could neither predict nor explain. I'll have time to look into it on Wednesday-ish. Meanwhile, if anyone has any ideas about what this could come from, they're welcome! Cheers, MS
Sign in to reply to this message.
> This fixes the SpanBar regression ID'd by Phil. Thanks for working on this. Ideally, this should be one commit. > It also fixes a few other things: 1) [...] 2) [...] 3) [...] Ideally, this should be another three different commits. Werner
Sign in to reply to this message.
On Oct 31, 2011, at 11:12 AM, Werner LEMBERG wrote: > >> This fixes the SpanBar regression ID'd by Phil. > > Thanks for working on this. Ideally, this should be one commit. > >> It also fixes a few other things: 1) [...] 2) [...] 3) [...] > > Ideally, this should be another three different commits. > > > Werner Hey Werner, The fixes are offshoots of the solution, meaning that the solution to this one problem serendipitously fixes the three others. Cheers, MS
Sign in to reply to this message.
>> Ideally, this should be another three different commits. > > The fixes are offshoots of the solution, meaning that the solution > to this one problem serendipitously fixes the three others. Ah, ok. Werner
Sign in to reply to this message.
----- Original Message ----- From: <mtsolo@gmail.com> To: <mtsolo@gmail.com> Cc: <reply@codereview.appspotmail.com>; <lilypond-devel@gnu.org> Sent: Monday, October 31, 2011 9:08 AM Subject: Fixes NoteColumn vs SpanBar collisions. (issue 5323062) > Reviewers: , > > Message: > Hey all, > > This fixes the SpanBar regression ID'd by Phil. It also fixes a few > other things: > > 1) The research I sent out about accidentals not hanging over barlines > (check my e-mails to the list from Oct. 6, Subject: First stab at > getting bar extents right) is actualized here. > 2) The elimination of skyline-vertical-padding corrects a horizontal > spacing issue where the cue-clef takes up too much space in > cue-clef-after-barline.ly and beam-collision-feasible-region.ly > 3) A new regtest, lyrics-pass-under-bar.ly, shows a bug fix for lyrics. > > The patch passes regtests (I have yet to do a full doc build). The only > concern I have is that a lot of regtests that combine tablature and > normal notation are effected but I cannot see how (lots of green, but > nothing looks like it moved). I don't know why my code is doing this, > and I'm always hesitant to push code that has an effect that I could > neither predict nor explain. I'll have time to look into it on > Wednesday-ish. Meanwhile, if anyone has any ideas about what this could > come from, they're welcome! > > Cheers, > MS > > Description: > Fixes NoteColumn vs SpanBar collisions. > > Please review this at http://codereview.appspot.com/5323062/ > > Affected files: > input/regression/lyrics-pass-under-bar.ly > M lily/include/pure-from-neighbor-interface.hh > M lily/pure-from-neighbor-engraver.cc > M lily/pure-from-neighbor-interface.cc > ly/engraver-init.ly > scm/define-grobs.scm > M scm/output-lib.scm Mike - do you have a Windows machine? And a Windows .exe for your changes? If so, you could run my pixel comparator. -- Phil Holmes
Sign in to reply to this message.
On Oct 31, 2011, at 3:19 PM, Phil Holmes wrote: > > Mike - do you have a Windows machine? And a Windows .exe for your changes? If so, you could run my pixel comparator. > > -- > Phil Holmes > > Unfortunately, I don't have a Windows machine, nor am I able to make a Windows .exe :( I'll be able to dig into the TabStaff regtests in a couple days to see why they're coming up green. Cheers, MS
Sign in to reply to this message.
On Oct 31, 2011, at 6:18 PM, mike@apollinemike.com wrote: > On Oct 31, 2011, at 3:19 PM, Phil Holmes wrote: > >> >> Mike - do you have a Windows machine? And a Windows .exe for your changes? If so, you could run my pixel comparator. >> >> -- >> Phil Holmes >> >> > > Unfortunately, I don't have a Windows machine, nor am I able to make a Windows .exe :( > I'll be able to dig into the TabStaff regtests in a couple days to see why they're coming up green. > > Cheers, > MS Figured it out. -) TabStaffs tend to make horizontal spacing tighter, as the average spring between paper columns has an ideal length that is shorter than that of staves comprised of normal notes. -) When the default skyline-horizontal-padding for NonMusicalPaperColumns was #0.6, this tightened spacing was blocked by the left-most NonMusicalPaperColumn. -) In the current patch, the NonMusicalPaperColumn has no skyline-horizontal-padding, which means that the spacing is not blocked and can compress even further. Whether or not the above scenario is aesthetically desirable or not is, of course, up for debate. Given that the 0.6 skyline horizontal padding is, according to the comment above it, only intended to block ledger lines from overlapping a bar line, I think that this blocking effect it has on horizontal spacing is an unintended side effect. So, unless anyone has arguments for why the old behavior should be kept, I think this is an acceptable (and even desirable) consequence of this patch. Cheers, MS
Sign in to reply to this message.
On 2011/11/01 08:25:48, mike_apollinemike.com wrote: > Given that the 0.6 skyline horizontal padding is, > according to the comment above it, only intended to block ledger lines from > overlapping a bar line, I think that this blocking effect it has on horizontal > spacing is an unintended side effect. So, unless anyone has arguments for why > the old behavior should be kept, I think this is an acceptable (and even > desirable) consequence of this patch. I have no clue about this issue at all, but reading this comment triggered my keyword-based interferometer. A principal reason for skyline horizontal padding was to avoid long stemmed notes from adjacent staffgroups from interlocking. While interlocking is usually desired between voices and reasonably appropriate inside of a pianostaff staffgroup with default clefs (!!!!), it is usually wrong between independent instruments and totally wrong between broken lines.
Sign in to reply to this message.
On Nov 1, 2011, at 9:44 AM, dak@gnu.org wrote: > On 2011/11/01 08:25:48, mike_apollinemike.com wrote: >> Given that the 0.6 skyline horizontal padding is, >> according to the comment above it, only intended to block ledger lines > from >> overlapping a bar line, I think that this blocking effect it has on > horizontal >> spacing is an unintended side effect. So, unless anyone has arguments > for why >> the old behavior should be kept, I think this is an acceptable (and > even >> desirable) consequence of this patch. > > I have no clue about this issue at all, but reading this comment > triggered my keyword-based interferometer. > > A principal reason for skyline horizontal padding was to avoid long > stemmed notes from adjacent staffgroups from interlocking. > > While interlocking is usually desired between voices and reasonably > appropriate inside of a pianostaff staffgroup with default clefs (!!!!), > it is usually wrong between independent instruments and totally wrong > between broken lines. Hey David, The skyline-padding in question only applies to NonMusicalPaperColumns, so it wouldn't have a holistic effect on staff groups. As for inter-lockitude, it looks like current master produces the same results as my patch for independent instruments (see attached - I don't see any difference between the two images). Perhaps its worth reporting this as a bug, but the solution would come from another region of LilyPond (probably pure heights of spanners). Cheers, MS
Sign in to reply to this message.
Passes Make but lots of reg test diffs here: http://lilypond-stuff.1065243.n5.nabble.com/Tracker-issue-2000-reg-test-diffs... James
Sign in to reply to this message.
I'll try to make a patch so that note-spacing is not affected by potential collisions between grobs from different staff-like-contexts (fixing the rare Lyrics-barline interaction). Then we can restore skyline-vertical-padding. Net, with this patch and the other SpanBarStub patch, you've added 680 lines of code containing two new engravers, to fix issue 910 and issue 1955. I don't like the thought of maintaining that. What is the overall plan for the new engravers? http://codereview.appspot.com/5323062/diff/12001/input/regression/lyrics-pass... File input/regression/lyrics-pass-under-bar.ly (right): http://codereview.appspot.com/5323062/diff/12001/input/regression/lyrics-pass... input/regression/lyrics-pass-under-bar.ly:4: texidoc = "Long lyrics should be allowed to pass under They do so, except in very rare cases in short examples where nothing protrudes from the staff. I agree it would be nice to change this, but did you really see the problem in real music? http://codereview.appspot.com/5323062/diff/12001/lily/pure-from-neighbor-engr... File lily/pure-from-neighbor-engraver.cc (right): http://codereview.appspot.com/5323062/diff/12001/lily/pure-from-neighbor-engr... lily/pure-from-neighbor-engraver.cc:32: vector<Grob *> pures_; This takes the lilypond-jargon 'pure' even further from its original inspiration (a pure function that does not depend on nor change program state). What is a 'pure' when used as a noun? http://codereview.appspot.com/5323062/diff/12001/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): http://codereview.appspot.com/5323062/diff/12001/scm/define-grob-properties.s... scm/define-grob-properties.scm:583: (neighbors-filtered ,boolean? "Callback to filter an element list.") But 'neighbors-filtered' is not the callback, but the resulting list, is it not? The question I look here to answer is "how does LilyPond behave differently if elements-filtered is changed?"
Sign in to reply to this message.
On Nov 3, 2011, at 11:39 PM, k-ohara5a5a@oco.net wrote: > I'll try to make a patch so that note-spacing is not affected by > potential collisions between grobs from different staff-like-contexts > (fixing the rare Lyrics-barline interaction). Then we can restore > skyline-vertical-padding. > > Net, with this patch and the other SpanBarStub patch, you've added 680 > lines of code containing two new engravers, to fix issue 910 and issue > 1955. I don't like the thought of maintaining that. > > What is the overall plan for the new engravers? > > Currently, LilyPond has no mechanism to calculate the Y-extents of Items in a relative way. You can't say "make the height of grob X such that it is always Y larger than that of grob Z." This is necessary for grobs (like barlines) that need to block certain grobs that fall above and below them. Idem for Staves, idem for time signatures. Currently, LilyPond accomplishes this through hardcoded values (checkout the extra-spacing-height for TimeSignature, for example), which potentially catches unwanted fuzz (a text script with '(0 . 0) for extra-spacing-width and low padding will be shifted over numerical time signatures but not common time). In general, I'd like to see the Pure_from_neighbor_engraver take care of the extra spacing heights for all grobs that are ordered by break alignments, as all of them should prevent overhang. This patch already does a good chunk of work for that. This engraver will also be essential for cross staff stems, which I plan to implement and which will need spacing stubs on the staves they cross whose heights can be calculated via this engraver. > http://codereview.appspot.com/5323062/diff/12001/input/regression/lyrics-pass... > File input/regression/lyrics-pass-under-bar.ly (right): > > http://codereview.appspot.com/5323062/diff/12001/input/regression/lyrics-pass... > input/regression/lyrics-pass-under-bar.ly:4: texidoc = "Long lyrics > should be allowed to pass under > They do so, except in very rare cases in short examples where nothing > protrudes from the staff. > > I agree it would be nice to change this, but did you really see the > problem in real music? > I see the problem in my music (which I consider real music!). > http://codereview.appspot.com/5323062/diff/12001/lily/pure-from-neighbor-engr... > File lily/pure-from-neighbor-engraver.cc (right): > > http://codereview.appspot.com/5323062/diff/12001/lily/pure-from-neighbor-engr... > lily/pure-from-neighbor-engraver.cc:32: vector<Grob *> pures_; > This takes the lilypond-jargon 'pure' even further from its original > inspiration (a pure function that does not depend on nor change program > state). > What is a 'pure' when used as a noun? > Grobs that are pure relevant. I'll use "pure_relevants_" instead. > http://codereview.appspot.com/5323062/diff/12001/scm/define-grob-properties.scm > File scm/define-grob-properties.scm (right): > > http://codereview.appspot.com/5323062/diff/12001/scm/define-grob-properties.s... > scm/define-grob-properties.scm:583: (neighbors-filtered ,boolean? > "Callback to filter an element list.") > But 'neighbors-filtered' is not the callback, but the resulting list, is > it not? > The question I look here to answer is "how does LilyPond behave > differently if elements-filtered is changed?" > It does nothing...this is bad property. I'll get rid of it tomorrow. Cheers, MS
Sign in to reply to this message.
"mike@apollinemike.com" <mike@apollinemike.com> writes: > On Nov 3, 2011, at 11:39 PM, k-ohara5a5a@oco.net wrote: > >> http://codereview.appspot.com/5323062/diff/12001/lily/pure-from-neighbor-engr... >> File lily/pure-from-neighbor-engraver.cc (right): >> >> http://codereview.appspot.com/5323062/diff/12001/lily/pure-from-neighbor-engr... >> lily/pure-from-neighbor-engraver.cc:32: vector<Grob *> pures_; >> This takes the lilypond-jargon 'pure' even further from its original >> inspiration (a pure function that does not depend on nor change program >> state). >> What is a 'pure' when used as a noun? >> > > Grobs that are pure relevant. I'll use "pure_relevants_" instead. Veto. "pure" sounds like inside jargon which one will tend to look up in the internals guide or wherever else one can expect to (and should!) find it. Lilypond "pure" apparently differs from common-sense "pure", but seems to roughly mean "an upper bound established without looking at line break decisions". While there _should_ be a word list of commonly used terms in our docs, one can figure that out, somewhat painfully, after looking at enough code. The grammatical barrier of nouning a verb or verbing a noun is tiny in comparison. "pure relevant" is gibberish. It again uses a Lilypond-specific "pure", but tacks on another common-use word in a meaning not usually employed. I have absolutely no idea what "grobs that are pure relevant" is supposed to mean. Not the fuzziest. -- David Kastrup
Sign in to reply to this message.
On Nov 4, 2011, at 1:02 AM, David Kastrup wrote: > "mike@apollinemike.com" <mike@apollinemike.com> writes: > >> On Nov 3, 2011, at 11:39 PM, k-ohara5a5a@oco.net wrote: >> >>> http://codereview.appspot.com/5323062/diff/12001/lily/pure-from-neighbor-engr... >>> File lily/pure-from-neighbor-engraver.cc (right): >>> >>> http://codereview.appspot.com/5323062/diff/12001/lily/pure-from-neighbor-engr... >>> lily/pure-from-neighbor-engraver.cc:32: vector<Grob *> pures_; >>> This takes the lilypond-jargon 'pure' even further from its original >>> inspiration (a pure function that does not depend on nor change program >>> state). >>> What is a 'pure' when used as a noun? >>> >> >> Grobs that are pure relevant. I'll use "pure_relevants_" instead. > > Veto. "pure" sounds like inside jargon which one will tend to look up > in the internals guide or wherever else one can expect to (and should!) > find it. Lilypond "pure" apparently differs from common-sense "pure", > but seems to roughly mean "an upper bound established without looking at > line break decisions". While there _should_ be a word list of commonly > used terms in our docs, one can figure that out, somewhat painfully, > after looking at enough code. The grammatical barrier of nouning a verb > or verbing a noun is tiny in comparison. > > "pure relevant" is gibberish. It again uses a Lilypond-specific "pure", > but tacks on another common-use word in a meaning not usually employed. > > I have absolutely no idea what "grobs that are pure relevant" is > supposed to mean. Not the fuzziest. David, Pure relevant (and pure-relevant) is used several places in the code (check out axis-group-interface.cc, define-grobs.scm, grob-property.cc). Are you suggesting that in a separate patch this nomenclature be rethought? If so, I think it's worth it to post a separate patch doing this. Cheers, MS
Sign in to reply to this message.
On Nov 4, 2011, at 1:52 AM, Keith OHara wrote: > On Fri, 04 Nov 2011 00:12:01 -0700, mike@apollinemike.com <mike@apollinemike.com> wrote: >> On Nov 3, 2011, at 11:39 PM, k-ohara5a5a@oco.net wrote: >>> What is the overall plan for the new engravers? >> >> Currently, LilyPond has no mechanism to calculate the Y-extents of Items in a relative way. You can't say "make the height of grob X such that it is always Y larger than that of grob Z." This is necessary for grobs (like barlines) that need to block certain grobs that fall above and below them. [The same] for Staves, [the same] for time signatures. Currently, LilyPond accomplishes this through hardcoded values (checkout the extra-spacing-height for TimeSignature, for example), which potentially catches unwanted fuzz (a text script with '(0 . 0) for extra-spacing-width and low padding will be shifted over numerical time signatures but not common time). > > Generally, though, there is a desired spacing from the Time Signature to the main note column, and we usually do not want decorations like accidentals or text scripts to influence that spacing, unless necessary to avoid (near) collisions. > > Issue 647 prompted the extra-spacing-height you mentioned, but that was for a (near) collision. Personally, I think the comment added to 'script-stack-horizontal.ly' was an overreaction. If the note with lots of decorations is well clear, I would rather have the decorations folded over the Clef or whatever. > I think that it is easier to tweak this sort of thing of bar lines know about the existence of leftward-hanging grobs and can deal with them. >> In general, I'd like to see the Pure_from_neighbor_engraver take care of the extra spacing heights for all grobs that are ordered by break alignments, as all of them should prevent overhang. > > Now, earlier you implied that change clefs were getting "too much space" in places where we blocked ledgers from overhanging: It's true that this patch conserves the old behavior in that regard with my most recent additions. I still stand by that, but I think the only way to make spacing more tweakable is, like I've been saying all along, to have grobs aware of the existence of others to the left and right. This is already accomplished via NoteSpacing and StaffSpacing, and the Pure_from_neighbor idea reinforces this. >> >> 2) The elimination of skyline-vertical-padding corrects a horizontal >> spacing issue where the cue-clef takes up too much space in >> cue-clef-after-barline.ly and beam-collision-feasible-region.ly >> > I'm assuming you changed your mind. The fix to get folded clefs correct reverts the old behavior, but again, I think that by working from this base, you can accomplish more. In general, responding to your initial question, the long term plan for this interface and engraver is to get grob heights relative with respect to other grobs. From this position, you can do more sophisticated spacing tweaking. > >>> I agree it would be nice to change this, but did you really see the >>> problem in real music? >> >> I see the problem in my music (which I consider real music!). >> > Wow. Are you a minimalist? > No. Check out http://www.apollinemike.com/first.pdf . This is one of the many pieces where these changes will be beneficial. > >>> What is a 'pure' when used as a noun? >> >> Grobs that are pure relevant. I'll use "pure_relevants_" instead. >> > Purely relevant to what? How can something be impurely relevant? Pure relevant is used all over the code. If you'd like to change it, I think that should be proposed by another patch. > > I know that you mean grobs that are relevant to the computation of an extra-spacing-height, for use in the note-spacing step, which comes before line-breaking, at which time we sometimes use approximate heights, which tend to be computed by functions that are (inaccurately) called 'pure'. > But, try to think of a less-jargony variable name, for the sake of this guy: <http://lists.gnu.org/archive/html/lilypond-devel/2011-05/msg00276.html> > The only reason that this guy was able to pull through was because he read the code and realized that, even though the meaning of pure was a stretch compared to the definition on Wikipedia, the naming conventions across LilyPond were consistent. This provides a sort of rosetta stone for the inquisitive mind. I would rather name something along the convention found in other places in the code, even if this convention can be rethought, than be unique in my naming. So, as I've said above & in an e-mail to David, I'm all for thinking out better ways to name variables/interfaces/engravers in the code, but it should be consistent. And, for now, I believe that my use of "pure relevant" is in keeping with how the phrase is used throughout the source code. Cheers, MS
Sign in to reply to this message.
On Nov 4, 2011, at 1:00 PM, Keith OHara wrote: > On Fri, 04 Nov 2011 09:47:16 -0700, mike@apollinemike.com <mike@apollinemike.com> wrote: > >> I think that it is easier to tweak this sort of thing of bar lines know about the existence of leftward-hanging grobs and can deal with them. > > I guess it is more flexible if there is the option to have 'extra-spacing-height be either > #,time-signature::extra-spacing-height or a pair like #'(-1 . 1) > Agreed, which is why I like growing this pure-from-neighbor infrastructure. It lends itself to this type of flexibility. >>>>> I agree it would be nice to change this, but did you really see the >>>>> problem in real music? >>>> >>>> I see the problem in my music (which I consider real music!). >>>> >>> Wow. Are you a minimalist? >>> >> No. Check out http://www.apollinemike.com/first.pdf . This is one of the many pieces where these changes will be beneficial. >> > I can't see where. The lyrics already successfully slide under bar lines in that piece. > Looking harder, I see no span bars, and no accidentals hanging over bar lines, no cross-staff stems. It's the lyrics. There are some very high notes where the lyrics don't tuck under the barline, which required me to make some extra-offset overrides (blech). > >>> Purely relevant to what? How can something be impurely relevant? >> >> Pure relevant is used all over the code. > Good point. There is even a predicate 'pure-relevant?' defining which grobs qualify. > Exactly. This is how the Pure_from_neighbor_engraver evaluates which grobs have pure height functions (line 49). Cheers, MS
Sign in to reply to this message.
Passes Make but lots of reg test diffs http://lilypond-stuff.1065243.n5.nabble.com/Tracker-issue-2000-reg-test-diffs... James
Sign in to reply to this message.
On Nov 4, 2011, at 2:35 PM, Keith OHara wrote: > The bug happens when the piece is entirely hight notes. (I should add some intelligence to handle this case, so that we can use 'skyline-vertical-padding without this annoyance.) Any single protrusion below the staff anywhere in the piece lets the lyrics slide under. > > \paper{ragged-right = ##t} > <<\new Staff \new Voice = "a" {c''2 g'' \break g''1 g''2 g''2} > \new Lyrics { \lyricsto "a" \lyricmode { he will go straight through } } >> > Why are you interested in keeping skyline-vertical-padding? I don't know the full history behind the creation/maintenance of the property, but it seems to me that this is a one-size-fits-all property that, if eliminated, would allow for more subtle solutions on a grob-to-grob basis. Currently, for example, the same property is causing ChordNames to expand measures if all the chords in a piece fall below the upper extreme of the staff (see chord-name-exceptinos.ly, for example). Furthermore, I'm always weary about magic #s that are based on an overridable layout decision. The existence of ledger lines for notes is not always guaranteed, and if they exist, they can be moved, in which case the 0.6 that's the current value would no longer work. Cheers, MS
Sign in to reply to this message.
> >> Pure relevant is used all over the code. > > > > Good point. There is even a predicate 'pure-relevant?' > > defining which grobs qualify. > > > > Exactly. This is how the Pure_from_neighbor_engraver > evaluates which grobs have > pure height functions (line 49). > http://codereview.appspot.com/5323062/diff/29001/lily/pure-from-neighbor-engr... File lily/pure-from-neighbor-engraver.cc (right): http://codereview.appspot.com/5323062/diff/29001/lily/pure-from-neighbor-engr... lily/pure-from-neighbor-engraver.cc:49: SCM pure_relevant_p = ly_lily_module_constant ("pure-relevant?"); Now, this fills 'items_' with things that pass the 'pure-relevant' test. (Since you say Item as opposed to Grob, do you exclude slurs and such?) http://codereview.appspot.com/5323062/diff/29001/lily/pure-from-neighbor-engr... lily/pure-from-neighbor-engraver.cc:60: pure_relevants_.push_back (i.item ()); But 'pure_relevants_' is something different from simply things that satisfy the predicate 'pure-relevant?'. It looks like it includes only items in the immediately-adjacent columns. This would seems to let accidentals on the second-neightbor-column cross the extended bar lines. How would this (untested) work? \new PianoStaff << \new Staff { R1*4 } \new Staff { e'1 | \acciaccatura e'16 <e'' cis''' dis'''>4 r2. | \slurUp \acciaccatura c''16 <c' fis gis>4 r2. <c' fis gis>4 r2. } >>
Sign in to reply to this message.
On Nov 5, 2011, at 4:11 PM, k-ohara5a5a@oco.net wrote: > http://codereview.appspot.com/5323062/diff/29001/lily/pure-from-neighbor-engr... > lily/pure-from-neighbor-engraver.cc:49: SCM pure_relevant_p = > ly_lily_module_constant ("pure-relevant?"); > Now, this fills 'items_' with things that pass the 'pure-relevant' > test. > (Since you say Item as opposed to Grob, do you exclude slurs and such?) > Yup. > http://codereview.appspot.com/5323062/diff/29001/lily/pure-from-neighbor-engr... > lily/pure-from-neighbor-engraver.cc:60: pure_relevants_.push_back > (i.item ()); > But 'pure_relevants_' is something different from simply things that > satisfy the predicate 'pure-relevant?'. It looks like it includes only > items in the immediately-adjacent columns. > It includes all columns. Immediately adjacent columns are assigned in Pure_from_neighbor_interface::keep_next_door_neighbors . > This would seems to let accidentals on the second-neightbor-column cross > the extended bar lines. How would this (untested) work? > > \new PianoStaff << > \new Staff { R1*4 } > \new Staff { > e'1 | \acciaccatura e'16 <e'' cis''' dis'''>4 r2. | > \slurUp \acciaccatura c''16 <c' fis gis>4 r2. > <c' fis gis>4 r2. > } >> You're right - good catch! I've put patch set up that fixes this. Cheers, MS
Sign in to reply to this message.
passes make, lots of reg test diffs here http://lilypond-stuff.1065243.n5.nabble.com/Tracker-issue-2000-reg-test-diffs... James
Sign in to reply to this message.
Until someone comes along who understands this, I'll display my confusions. http://codereview.appspot.com/5323062/diff/28002/lily/pure-from-neighbor-inte... File lily/pure-from-neighbor-interface.cc (right): http://codereview.appspot.com/5323062/diff/28002/lily/pure-from-neighbor-inte... lily/pure-from-neighbor-interface.cc:84: // uses drul array for grace (LEFT) vs. not grace (RIGHT). Why use a drul array instead of just an array from 0 (false) to 1 (true) indexed by your boolean 'has_grace' ? http://codereview.appspot.com/5323062/diff/28002/lily/pure-from-neighbor-inte... lily/pure-from-neighbor-interface.cc:96: && (grace == LEFT ? has_grace : !has_grace)) In fact, why even use a loop over grace-ness if the actions within the loop occur only for one of two passes through ? http://codereview.appspot.com/5323062/diff/28002/lily/pure-from-neighbor-inte... lily/pure-from-neighbor-interface.cc:113: if (d * srl_drul[LEFT][d] < d * srl_drul[RIGHT][d]) This seems to say "if the closest grace-note-column is closer than the closest non-grace-column, include the stuff in the grace-note-column in my 'neighbors'". Why are you treating grace-note columns specially ?
Sign in to reply to this message.
http://codereview.appspot.com/5323062/diff/28002/lily/pure-from-neighbor-inte... File lily/pure-from-neighbor-interface.cc (right): http://codereview.appspot.com/5323062/diff/28002/lily/pure-from-neighbor-inte... lily/pure-from-neighbor-interface.cc:84: // uses drul array for grace (LEFT) vs. not grace (RIGHT). On 2011/11/07 01:25:30, Keith wrote: > Why use a drul array instead of just an array from 0 (false) to 1 (true) indexed > by your boolean 'has_grace' ? This is a good idea - will do. http://codereview.appspot.com/5323062/diff/28002/lily/pure-from-neighbor-inte... lily/pure-from-neighbor-interface.cc:96: && (grace == LEFT ? has_grace : !has_grace)) On 2011/11/07 01:25:30, Keith wrote: > In fact, why even use a loop over grace-ness if the actions within the loop > occur only for one of two passes through ? I'm not sure what you mean...the actions apply for both passes. http://codereview.appspot.com/5323062/diff/28002/lily/pure-from-neighbor-inte... lily/pure-from-neighbor-interface.cc:113: if (d * srl_drul[LEFT][d] < d * srl_drul[RIGHT][d]) On 2011/11/07 01:25:30, Keith wrote: > This seems to say "if the closest grace-note-column is closer than the closest > non-grace-column, include the stuff in the grace-note-column in my 'neighbors'". > Why are you treating grace-note columns specially ? This is saying "if the closest column is a grace-note-column, include the next-closest column as well." This gets rid of any accidental overlap problems at the expense of potentially adding a little extra vertical space to the page-spacer calculations, but this extra space seems to be so minimal as to not be a problem (not unlike pure heights for beamed stems, which could also slightly overshoot their actual height).
Sign in to reply to this message.
On Nov 7, 2011, at 10:46 AM, Keith OHara wrote: > >> This is saying "if the closest column is a grace-note-column, include >> the next-closest column as well." This gets rid of any accidental >> overlap problems at the expense of potentially adding a little extra >> vertical space to the page-spacer calculations, but this extra space >> seems to be so minimal as to not be a problem (not unlike pure heights >> for beamed stems, which could also slightly overshoot their actual >> height). > > I had not realized that the results of the pure-from-neighbor system influence the page-spacing. This is inconvenient. Now I have some idea why there is so much effort to increase the height only as much as needed. > I misspoke - in this case, it does not. But it does potentially add unwanted horizontal space. All pure heights influence page-spacing. Try: \relative c' { \override Staff . BarLine #'Y-extent = #(ly:make-unpure-pure-container '(-1000000 . 100000) '(-2 . 2)) \repeat unfold 52 { a b c d } } extra-spacing-height, which is what this current patch adds, has no influence on vertical spacing. But there is so much effort to increase the height as needed for the following reason. CASE 1 \new GrandStaff << \new Staff \relative c { \override Staff . BarLine #'extra-spacing-height = #'(-10000 . 10000) \override TextScript #'extra-spacing-width = ##f \repeat unfold 52 { dis4 dis dis dis^"foo bar" } } \new Staff \repeat unfold 52 R1 >> Build this with current master and you'll see the span bar problem fixed but the text script elongating the measure. CASE TWO \new GrandStaff << \new Staff \relative c { \override TextScript #'extra-spacing-width = ##f \repeat unfold 52 { dis4 dis dis dis^"foo bar" } } \new Staff \repeat unfold 52 R1 >> Build this with the most current version of my patch and the text script won't elongate the measure (it did with older versions of my patch, but your comments made me realize a flaw in the way the interface worked, so thank you!). In a future patch, I can imagine the creation of a new grob, SpacingStub, that replaces extra spacing height. Two would be attached to every grob, and they would allow for more fine-tuned break alignment. For example, to achieve accidentals that don't cross over barlines but are allowed to get snug like the examples you added to the tracker, the SpacingStub could have a different spacing-alist than BarLine to allow this to happen. Cheers, MS
Sign in to reply to this message.
Passes make but lots of reg test diffs as usual http://lilypond-stuff.1065243.n5.nabble.com/Tracker-issue-2000-reg-test-diffs... Jame
Sign in to reply to this message.
On Nov 7, 2011, at 8:19 PM, Keith OHara wrote: > Mike, > Now that we understand that we can adjust 'extra-spacing-height > for note-spacing, without messing up line-breaking or page-spacing, > let's revisit the big picture. > > On Fri, 04 Nov 2011 00:12:01 -0700, mike@apollinemike.com <mike@apollinemike.com> wrote: > >> On Nov 3, 2011, at 11:39 PM, k-ohara5a5a@oco.net wrote: >> >>> What is the overall plan for the new engravers? >>> >> Currently, LilyPond has no mechanism to calculate the Y-extents of Itemsin a relative way. You can't say "make the height of grob X such thatit is always Y larger than that of grob Z." > > So far, so good. > >> This is necessary for grobs (like barlines) that need to block certain grobs >> that fall above and below them. Idem for Staves, idem for time signatures.Currently, LilyPond accomplishes this through hardcoded values (check out >> the extra-spacing-height for TimeSignature, for example), which potentially >> catches unwanted fuzz (a text script with '(0 . 0) for extra-spacing-width >> and low padding will be shifted over numerical time signatures but notcommon time). > > Well, it depends on /why/ we need the blocking. > If the goal is to avoid collisions, we use the extents plus extra-spacing-* > For horizontal spacing based on the meaning of the grobs, we have the 'space-alist. > Grobs that need prominence relative to their non-colliding neighbors can benefit > from your pure-from-neighbor-interface. > > Span Bars need space to avoid collisions. > > Separating SpanBars into segments, as you did for issue 910, seems wise because > in the case of issue 910 they can print as segments. > I'm OK with getting rid of all of this pure-from-neighbor and span-bar-stub stuff and just create several SpanBars instead of one SpanBar with gaps in its stencil. I can reuse code I've already written, so it's not that bad. But, my question is: was it ever a good idea to have the pure heights of span bars calculated the way they were? SpanBars are inherently cross staff, so how can they estimate a correct Y-extent without triggering a VerticalAlignment? Cheers, MS
Sign in to reply to this message.
On Nov 7, 2011, at 9:33 PM, Keith OHara wrote: > On Mon, 07 Nov 2011 20:51:14 -0800, mike@apollinemike.com <mike@apollinemike.com> wrote: > >> I'm OK with getting rid of all of this pure-from-neighbor and span-bar-stub stuff and just create several SpanBars instead of one SpanBar with gaps in its stencil. I can reuse code I've already written, so it's not that bad. > > Would you do it as a revert of 4f49b000 followed by a commit to make a clean fix for issue 910 ? > Exactly. I'll have time to work on this today. >> But, my question is: was it ever a good idea to have the pure heights of span bars calculated the way they were? SpanBars are inherently cross staff, so how can they estimate a correct Y-extent without triggering a VerticalAlignment? >> > > The correct height for SpanBars during note-spacing would be whatever corresponds to the tentative staff-spacing used during horizontal spacing -- the spacing table for which you recently implemented caching. SpanBars did have that as their Y-extent in 2.14. (But of course they did not have the proper gaps in their skylines, if somebody used 'allow-span-bar=#f) > Ok.
Sign in to reply to this message.
On Nov 8, 2011, at 4:20 AM, mike@apollinemike.com wrote: > On Nov 7, 2011, at 9:33 PM, Keith OHara wrote: > >> On Mon, 07 Nov 2011 20:51:14 -0800, mike@apollinemike.com <mike@apollinemike.com> wrote: >> >>> I'm OK with getting rid of all of this pure-from-neighbor and span-bar-stub stuff and just create several SpanBars instead of one SpanBar with gaps in its stencil. I can reuse code I've already written, so it's not that bad. >> >> Would you do it as a revert of 4f49b000 followed by a commit to make a clean fix for issue 910 ? >> > > Exactly. I'll have time to work on this today. So, I checked what'd happen if the patch reverted and it reverts fine (it needs one additional revert - it's relatively easy to swim upstream with this one). I also tried to think as lucidly as I can about how this works and what is best from a design perspective and, after mulling over it some more, I've uploaded a new patch that simplifies the interface as much as possible and fixes (I think) all of the issues I've raised. I've also uploaded some doc text for review that explains how I understand pure properties (which hopefully highly correlates with how they actually work). It was after writing this text that I realized how to formulate the problem as succinctly as possible with a small lily example. I've attached two versions of: \relative c'''' { <ais bis cis dis> } one from current master and one from the most recent version of my patch set. If the version from current master is acceptable in traditional typesetting, then I'll revert. But all the (meager) evidence I find is to the contrary (also see attached), which is why I'm standing by the most recent patch set. Cheers, MS
Sign in to reply to this message.
Passes make and all reg test diffs here: http://lilypond-stuff.1065243.n5.nabble.com/Tracker-issue-2000-reg-test-diffs... james
Sign in to reply to this message.
"mike@apollinemike.com" <mike@apollinemike.com> writes: > example. I've attached two versions of: > > \relative c'''' { > <ais bis cis dis> > } > > > one from current master and one from the most recent version of my > patch set. If the version from current master is acceptable in > traditional typesetting, then I'll revert. But all the (meager) > evidence I find is to the contrary (also see attached), which is why > I'm standing by the most recent patch set. Actually, I consider _both_ versions awful. When note heads and accidentals each form a separate 2D cluster, the topography of the clusters needs to be strongly related, or you don't have a chance to unravel the correspondences when sightreading. -- David Kastrup
Sign in to reply to this message.
http://codereview.appspot.com/5323062/diff/39005/lily/pure-from-neighbor-engr... File lily/pure-from-neighbor-engraver.cc (right): http://codereview.appspot.com/5323062/diff/39005/lily/pure-from-neighbor-engr... lily/pure-from-neighbor-engraver.cc:60: pure_relevants_.push_back (i.item ()); It seems that pure_relevants_[] are the grobs that have the pure-from-neighbor interface, which would be the BarLines, Clefs, SpanBarStubs etc. This is a disjoint set from item_[]. All of item_[] satisfy (pure-relevant? x) Not all of pure_relevants_[] satisfy (pure-relevant? x) The variable naming is evil. http://codereview.appspot.com/5323062/diff/39005/scm/define-grobs.scm File scm/define-grobs.scm (right): http://codereview.appspot.com/5323062/diff/39005/scm/define-grobs.scm#newcode... scm/define-grobs.scm:1743: (pure-from-neighbor-ignore . #t) Scripts can be wide enough to hit span bars. What good does pure-from-neighbor-ignore do ? The behavior is surprising. Playing with the example you gave earlier : \new GrandStaff << \new Staff R1*5 \new Staff { \override TextScript #'extra-spacing-width = ##f dis4 dis dis dis^"sliding-over" | dis4 dis dis dis | dis'''4 dis dis dis^"sliding-over-not" | R1 c'''4\espressivo a'2. } >>
Sign in to reply to this message.
On Nov 8, 2011, at 10:16 PM, k-ohara5a5a@oco.net wrote: > > http://codereview.appspot.com/5323062/diff/39005/lily/pure-from-neighbor-engr... > File lily/pure-from-neighbor-engraver.cc (right): > > http://codereview.appspot.com/5323062/diff/39005/lily/pure-from-neighbor-engr... > lily/pure-from-neighbor-engraver.cc:60: pure_relevants_.push_back > (i.item ()); > It seems that pure_relevants_[] are the grobs that have the > pure-from-neighbor interface, which would be the BarLines, Clefs, > SpanBarStubs etc. > This is a disjoint set from item_[]. > All of item_[] satisfy (pure-relevant? x) > Not all of pure_relevants_[] satisfy (pure-relevant? x) > > The variable naming is evil. You're right...brain fart on my part. Fixed. > > http://codereview.appspot.com/5323062/diff/39005/scm/define-grobs.scm > File scm/define-grobs.scm (right): > > http://codereview.appspot.com/5323062/diff/39005/scm/define-grobs.scm#newcode... > scm/define-grobs.scm:1743: (pure-from-neighbor-ignore . #t) > Scripts can be wide enough to hit span bars. > What good does pure-from-neighbor-ignore do ? > The behavior is surprising. Playing with the example you gave earlier > : > \new GrandStaff << > \new Staff R1*5 > \new Staff { > \override TextScript #'extra-spacing-width = ##f > dis4 dis dis dis^"sliding-over" | > dis4 dis dis dis | > dis'''4 dis dis dis^"sliding-over-not" | R1 > c'''4\espressivo a'2. } >> > You're also right. I blame lack of sleep. I got rid of this property and now everything plays as expected. Cheers, MS
Sign in to reply to this message.
passes make and reg test diffs are all here: http://lilypond-stuff.1065243.n5.nabble.com/Tracker-issue-2000-reg-test-diffs... James
Sign in to reply to this message.
http://codereview.appspot.com/5323062/diff/53017/lily/pure-from-neighbor-engr... File lily/pure-from-neighbor-engraver.cc (right): http://codereview.appspot.com/5323062/diff/53017/lily/pure-from-neighbor-engr... lily/pure-from-neighbor-engraver.cc:72: //first, clump pure_relevants into vectors of grobs that have the same column. comment update http://codereview.appspot.com/5323062/diff/53017/lily/pure-from-neighbor-engr... lily/pure-from-neighbor-engraver.cc:93: to the pure grobs on either side. comment update http://codereview.appspot.com/5323062/diff/53017/lily/span-bar-stub-engraver.cc File lily/span-bar-stub-engraver.cc (right): http://codereview.appspot.com/5323062/diff/53017/lily/span-bar-stub-engraver.... lily/span-bar-stub-engraver.cc:55: // note that this can get out of hand if there are lots of vertical axis groups... You are telling me to worry but not telling me what to worry about. There is one Span_bar_stub_engraver per Staff, and for each of these you add one grob for each spanbar. The total number is typically the same as the number of BarLines, so I do not see what gets "out of hand". http://codereview.appspot.com/5323062/diff/53017/scm/define-grobs.scm File scm/define-grobs.scm (right): http://codereview.appspot.com/5323062/diff/53017/scm/define-grobs.scm#newcode201 scm/define-grobs.scm:201: (extra-spacing-height . ,pure-from-neighbor-interface::extra-spacing-height) Now this includes all Items within one bar (usually) on each side. But extra-spacing-height is used when the tentative staff-spacing is estimated using boxes around everything in the entire length of each staff, the so-called full-score-pure-minimum-translations. They are minimum in the sense of compressed springs, but not really very minimum because the bounding boxes are so conservative. If we know the space allocated to a staff during horizontal spacing (the ly:axis-group-interface::height, I think) and the goal (for issue 1955) is to build a wall to block anything on the staff, then why build each wall to a custom, usually shorter, height. http://codereview.appspot.com/5323062/diff/53017/scm/define-grobs.scm#newcode... scm/define-grobs.scm:1859: (SpanBarStub I can see these have effect in Lyrics, but not in regular Staves. Weren't they intended to perform the horizontal spacing functions of SpanBar ? They do not affect the skylines of the NonMusicalPaperColumn. Here, I add some padding so we can see the PaperColumn outlines, and remove the BarLines from the PaperColumn outline so we can see more easily. I see the vestigial SpanBars at the top of the page (I widened them to distinguish them) but no SpanBarStub, and nothing even trying to take up space now that the BarLine is out of the picture. \layout { \context { \Score \override NonMusicalPaperColumn #'stencil = #ly:separation-item::print \override PaperColumn #'stencil = #ly:separation-item::print \override NonMusicalPaperColumn #'skyline-vertical-padding = #0.5 \override BarLine #'extra-spacing-width = #'(+inf.0 . -inf.0) \override SpanBar #'extra-spacing-width = #'(-1.0 . 1.0) }} #(ly:set-option 'debug-skylines) \new GrandStaff << \new Staff { dis'''1 | dis'''4 r2. | c''1 } \new Staff { e'1 | e'1 | fis'''4 r2. } >>
Sign in to reply to this message.
As always, many thanks for your comments! http://codereview.appspot.com/5323062/diff/53017/lily/pure-from-neighbor-engr... File lily/pure-from-neighbor-engraver.cc (right): http://codereview.appspot.com/5323062/diff/53017/lily/pure-from-neighbor-engr... lily/pure-from-neighbor-engraver.cc:72: //first, clump pure_relevants into vectors of grobs that have the same column. On 2011/11/10 07:04:56, Keith wrote: > comment update Done. http://codereview.appspot.com/5323062/diff/53017/lily/pure-from-neighbor-engr... lily/pure-from-neighbor-engraver.cc:93: to the pure grobs on either side. On 2011/11/10 07:04:56, Keith wrote: > comment update Done. http://codereview.appspot.com/5323062/diff/53017/lily/span-bar-stub-engraver.cc File lily/span-bar-stub-engraver.cc (right): http://codereview.appspot.com/5323062/diff/53017/lily/span-bar-stub-engraver.... lily/span-bar-stub-engraver.cc:55: // note that this can get out of hand if there are lots of vertical axis groups... On 2011/11/10 07:04:56, Keith wrote: > You are telling me to worry but not telling me what to worry about. There is > one Span_bar_stub_engraver per Staff, and for each of these you add one grob for > each spanbar. The total number is typically the same as the number of BarLines, > so I do not see what gets "out of hand". You're right - removed. http://codereview.appspot.com/5323062/diff/53017/scm/define-grobs.scm File scm/define-grobs.scm (right): http://codereview.appspot.com/5323062/diff/53017/scm/define-grobs.scm#newcode201 scm/define-grobs.scm:201: (extra-spacing-height . ,pure-from-neighbor-interface::extra-spacing-height) On 2011/11/10 07:04:56, Keith wrote: > Now this includes all Items within one bar (usually) on each side. > > But extra-spacing-height is used when the tentative staff-spacing is estimated > using boxes around everything in the entire length of each staff, the so-called > full-score-pure-minimum-translations. They are minimum in the sense of > compressed springs, but not really very minimum because the bounding boxes are > so conservative. > > If we know the space allocated to a staff during horizontal spacing (the > ly:axis-group-interface::height, I think) and the goal (for issue 1955) is to > build a wall to block anything on the staff, then why build each wall to a > custom, usually shorter, height. Because the wall needs to extend places where span bars don't, namely above and below the staff. And, as we've said before, there can be holes in the wall. One strategy is to create multiple span bars for one column depending on where these holes are (this would make overrides more difficult, but there are ways to get around it that could work not unlike glissando-index). However, this does not fix the fundamental problem of how to deal with grobs that span bars would never hit (i.e. accidentals) but that shouldn't hang over barlines, time signatures, clefs, etc. So, using your metaphor of building a wall, I think that the idea of a custom wall is more tractable - it both allows for holes (span-bar-allow-span-bar.ly) and allows for the blocking power of the wall to kick in where it is not visibly present (i.e. the scenario with accidentals described above). The quick fix is obviously to revert the patch, but I think we are close with this, and as I've said several times before, if a rule in typesetting is that no objects to the right of object X should ever pass over or under object X, it makes sense that object X's height, in some way, be estimated from the neighbors that it is supposed to block. http://codereview.appspot.com/5323062/diff/53017/scm/define-grobs.scm#newcode... scm/define-grobs.scm:1859: (SpanBarStub On 2011/11/10 07:04:56, Keith wrote: > I can see these have effect in Lyrics, but not in regular Staves. Weren't they > intended to perform the horizontal spacing functions of SpanBar ? They do not > affect the skylines of the NonMusicalPaperColumn. > > Here, I add some padding so we can see the PaperColumn outlines, and remove the > BarLines from the PaperColumn outline so we can see more easily. I see the > vestigial SpanBars at the top of the page (I widened them to distinguish them) > but no SpanBarStub, and nothing even trying to take up space now that the > BarLine is out of the picture. > > \layout { \context { \Score > \override NonMusicalPaperColumn #'stencil = #ly:separation-item::print > \override PaperColumn #'stencil = #ly:separation-item::print > \override NonMusicalPaperColumn #'skyline-vertical-padding = #0.5 > \override BarLine #'extra-spacing-width = #'(+inf.0 . -inf.0) > \override SpanBar #'extra-spacing-width = #'(-1.0 . 1.0) > }} #(ly:set-option 'debug-skylines) > \new GrandStaff << > \new Staff { dis'''1 | dis'''4 r2. | c''1 } > \new Staff { e'1 | e'1 | fis'''4 r2. } >> The Span_bar_stub engraver exists only in the Lyrics context. It is intended to perform the horizontal spacing functions of SpanBar in vertical axis groups that SpanBars traverse and that are supposed to be effected by SpanBars but do not have BarLines. With respect to your example above, Staves would not need them, as they have BarLines. If you add a Bar_line_engraver to a Lyrics context, you should remove the Span_bar_engraver. If BarLines are out of the picture but you want them to take up space as they did before, it seems like BarLine #'transparent is the way to go.
Sign in to reply to this message.
On 2011/11/10 14:51:22, MikeSol wrote: >> If we know the space allocated to a staff during horizontal spacing > (the >> ly:axis-group-interface::height, I think) and the goal (for issue > 1955) is to >> build a wall to block anything on the staff, then why build each wall > to a >> custom, usually shorter, height. > > Because the wall needs to extend places where span bars don't, namely > above and below the staff. To fix issue 1955, they do need that, but I thought every wall on a given staff could be the same height, specifically the height tentatively allocated to the staff and its contents during horizontal spacing. I was talking about custom heights for every bar along the Staff, rather than customizing different heights for different staves. > And, as we've said before, there can be > holes in the wall. One strategy is to create multiple span bars for one > column depending on where these holes are (this would make overrides > more difficult, but there are ways to get around it that could work not > unlike glissando-index). Well, if we \once\override Score.SpanBar that should affect the whole column; while \override Staff.SpanBar{Stub} could affect just one staff. > However, this does not fix the fundamental > problem of how to deal with grobs that span bars would never hit (i.e. > accidentals) but that shouldn't hang over barlines, time signatures, > clefs, etc. > You do not need to address multiple issues at one. If SpanBars have space reserved where and only where they print, issue 910 would be solved. If BarLines and KeySignatures later receive extra-spacing-height to prevent accidentals from hanging over, issue 1955 would be solved. Separation has the advantage that, should there be an exception to the rules exemplified by either of these issues, a user can override one rule and keep the other. > So, using your metaphor of building a wall, I think that the idea of a > custom wall is more tractable - it both allows for holes > (span-bar-allow-span-bar.ly) and allows for the blocking power of the > wall to kick in where it is not visibly present (i.e. the scenario with > accidentals described above). >
Sign in to reply to this message.
On Nov 10, 2011, at 10:38 AM, k-ohara5a5a@oco.net wrote: > On 2011/11/10 14:51:22, MikeSol wrote: > >>> If we know the space allocated to a staff during horizontal spacing >> (the >>> ly:axis-group-interface::height, I think) and the goal (for issue >> 1955) is to >>> build a wall to block anything on the staff, then why build each wall >> to a >>> custom, usually shorter, height. > >> Because the wall needs to extend places where span bars don't, namely >> above and below the staff. > > To fix issue 1955, they do need that, but I thought every wall on a > given staff could be the same height, specifically the height > tentatively allocated to the staff and its contents during horizontal > spacing. > > I was talking about custom heights for every bar along the Staff, rather > than customizing different heights for different staves. > Ah, gotchya. I think the problem that this'd cause is that the overestimation may block lyrics/dynamics/what-have-you from sliding to the left as much as they could. I can certainly give this a shot, though. >> And, as we've said before, there can be >> holes in the wall. One strategy is to create multiple span bars for > one >> column depending on where these holes are (this would make overrides >> more difficult, but there are ways to get around it that could work > not >> unlike glissando-index). > > Well, if we \once\override Score.SpanBar that should affect the whole > column; while \override Staff.SpanBar{Stub} could affect just one staff. > SpanBarStubs only exist in contexts that SpanBars traverse but do not have bar lines (see my e-mail on the subject from this morning - it was in a different thread). So, this wouldn't work unless they were redesigned to be a representation of the span bar in all contexts. The problem I see with going down this road is that it wouldn't solve the problem of bar line overhang when span bars are not present. >> However, this does not fix the fundamental >> problem of how to deal with grobs that span bars would never hit (i.e. >> accidentals) but that shouldn't hang over barlines, time signatures, >> clefs, etc. > > > You do not need to address multiple issues at one. > And I would even go so far as to say that one shouldn't address multiple issues at once unless absolutely necessary. But here, I think that we can find one elegant solution that solves everything instead of several separate solutions that cause code and logic duplication. > If SpanBars have space reserved where and only where they print, issue > 910 would be solved. > > If BarLines and KeySignatures later receive extra-spacing-height to > prevent accidentals from hanging over, issue 1955 would be solved. > > Separation has the advantage that, should there be an exception to the > rules exemplified by either of these issues, a user can override one > rule and keep the other. > I'll do my best to separate these out - as I said above, I don't want to do it if it is going to compromise the universality of the solution and/or if it'll require reverting one solution to implement a later one. But, I'll mull over it and see if I can cook something up on my 11 hour plane ride today (uggggghhhhhhhhhhhhh). > >> So, using your metaphor of building a wall, I think that the idea of a >> custom wall is more tractable - it both allows for holes >> (span-bar-allow-span-bar.ly) and allows for the blocking power of the >> wall to kick in where it is not visibly present (i.e. the scenario > with >> accidentals described above). > > > http://codereview.appspot.com/5323062/
Sign in to reply to this message.
passes make and reg tests all posted here: http://lilypond-stuff.1065243.n5.nabble.com/Tracker-issue-2000-reg-test-diffs... James
Sign in to reply to this message.
On Nov 10, 2011, at 7:49 PM, mike@apollinemike.com wrote: > On Nov 10, 2011, at 10:38 AM, k-ohara5a5a@oco.net wrote: > > >> If SpanBars have space reserved where and only where they print, issue >> 910 would be solved. >> >> If BarLines and KeySignatures later receive extra-spacing-height to >> prevent accidentals from hanging over, issue 1955 would be solved. >> >> Separation has the advantage that, should there be an exception to the >> rules exemplified by either of these issues, a user can override one >> rule and keep the other. >> > > I'll do my best to separate these out - as I said above, I don't want to do it if it is going to compromise the universality of the solution and/or if it'll require reverting one solution to implement a later one. But, I'll mull over it and see if I can cook something up on my 11 hour plane ride today (uggggghhhhhhhhhhhhh). > I posted an uber-patch online that can be separated out into several patches. This uses a new extra-spacing-height for barline to allow span bars to block offending grobs while not applying this to barlines that either don't have span bars or have allow-span-bar set to ##f. I think that this does everything that needs to be done to fix regressions in addition to adding new functionality that makes sure that accidentals don't shift over/under time signatures, clefs, and key signatures. After ironing all the kinks out (assuming that there are still kinks to iron out), my plan would be to do the following: 1) Push a patch that fixes the span bar problem. 2) Push a patch that adds extra spacing height to clefs, key signatures, and time signatures to block accidentals from trailing over them. 3) Push a patch that makes the default for LilyPond to block accidentals trailing over barlines. 2 and 3 are still up for debate, but as I've said several times in this thread, there is nothing that i've found in the literature to suggest that music does not work this way. Cheers, MS
Sign in to reply to this message.
Mike, On Fri, Nov 11, 2011 at 3:34 PM, mike@apollinemike.com <mike@apollinemike.com> wrote: > ... > I posted an uber-patch online that can be separated out into several > patches. .... My only concern is that it will only get tested as an uber patch and unless you are going to push it all at once (albeit with several revertable commits) we should test each patch individually. -- -- James
Sign in to reply to this message.
On Nov 11, 2011, at 4:44 PM, Peekay Ex wrote: > Mike, > > On Fri, Nov 11, 2011 at 3:34 PM, mike@apollinemike.com > <mike@apollinemike.com> wrote: >> > > ... > >> I posted an uber-patch online that can be separated out into several >> patches. > > .... > > My only concern is that it will only get tested as an uber patch and > unless you are going to push it all at once (albeit with several > revertable commits) we should test each patch individually. > Hey James, I see what you're saying - because all of these functions are intertwined, I want to make sure that they get reviewed together. Keith had asked me in an earlier e-mail to explain where I see the pure-from-neighbor-interface going, and I think it's important that one can grok its global usefulness so that one can evaluate if it is ultimately worth it to grow it to the size to which this patch would grow it or if it should be scrapped. After we finish that discussion, I'll post separate patches (in sequence) for each change. Cheers, MS
Sign in to reply to this message.
Mike, On Fri, Nov 11, 2011 at 3:55 PM, mike@apollinemike.com <mike@apollinemike.com> wrote: > On Nov 11, 2011, at 4:44 PM, Peekay Ex wrote: > >> Mike, >> >> On Fri, Nov 11, 2011 at 3:34 PM, mike@apollinemike.com >> <mike@apollinemike.com> wrote: >>> >> >> ... >> >>> I posted an uber-patch online that can be separated out into several >>> patches. >> >> .... >> >> My only concern is that it will only get tested as an uber patch and >> unless you are going to push it all at once (albeit with several >> revertable commits) we should test each patch individually. >> > > Hey James, > > I see what you're saying - because all of these functions are intertwined, I want to make sure that they get reviewed together. Keith had asked me in an earlier e-mail to explain where I see the pure-from-neighbor-interface going, and I think it's important that one can grok its global usefulness so that one can evaluate if it is ultimately worth it to grow it to the size to which this patch would grow it or if it should be scrapped. > > After we finish that discussion, I'll post separate patches (in sequence) for each change. > > Cheers, > MS > > Thanks, so just to be clear i can still test the patch en masse? -- -- James
Sign in to reply to this message.
On Nov 11, 2011, at 5:22 PM, Peekay Ex wrote: > > Thanks, so just to be clear i can still test the patch en masse? > > -- > -- > James Yup. Cheers, MS
Sign in to reply to this message.
Doesn't pass make --snip-- [/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/lily-sort.scm] [/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-functions.scm] [/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-translation.scm] [/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-music.scm] [/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-type-predicates.scm] [/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-identifiers.scm] [/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-context-mods.scm] [/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-backend.scm fatal error: define-grob-properties.scm: cannot find interface for property: has-span-bar make[1]: *** [out/internals.texi] Error 1 make[1]: Leaving directory `/home/jlowe/lilypond-git/build/Documentation' make: *** [all] Error 2 jlowe@jlowe-lilybuntu2:~/lilypond-git/build$ --snip- James
Sign in to reply to this message.
Passes Make and reg tests are all here http://lilypond-stuff.1065243.n5.nabble.com/Tracker-issue-2000-12-November-td... James
Sign in to reply to this message.
Passes make and reg tests are here http://code.google.com/p/lilypond/issues/detail?id=2000#c32 James
Sign in to reply to this message.
Looks okay to me. The ghost of the SpanBar (left over from the issue 910 patch) sometimes has an effect \new GrandStaff << \new Staff { e'1 | eses'''4 r2. | e'1 } \new Staff { e'1 | e'1 | eses4 r2. } >> Something like \override Score.SpanBar #'extra-spacing-height = #'(+inf.0 . -inf.0) takes care of that problem. On 2011/11/10 18:49:48, mike_apollinemike.com wrote: > On Nov 10, 2011, at 10:38 AM, mailto:k-ohara5a5a@oco.net wrote: > > > I was talking about custom heights for every bar along the Staff, > > I think the problem that this'd cause is that the overestimation may block > lyrics/dynamics/what-have-you from sliding to the left as much as they could. Well, any overestimation in your blocking heights also becomes an overestimation in the tentative space between Staves/Lyrics/Dynamics at the horizontal spacing step. For example, the notoriously-bad pure-height estimates for slurs cause the tentative-spacing to be looser. Thus a slur (even on a later line or later page) will cause Lily to think the Lyrics might have to move away, and thus avoid the sticky lyrics effect: \paper {ragged-right = ##t} \relative c''' { c2 c c c \break c( g,) } \addlyrics { foo bar foooooooo bar } Use of custom locally-determined heights for each bar line seems unnecessarily complicated (but not as bad as the grace_fixup_list so I won't complain). Run fixcc.py on the changed .cc files to clean up some whitespace problems. http://codereview.appspot.com/5323062/diff/53040/input/regression/lyrics-pass... File input/regression/lyrics-pass-under-bar.ly (right): http://codereview.appspot.com/5323062/diff/53040/input/regression/lyrics-pass... input/regression/lyrics-pass-under-bar.ly:4: texidoc = "Long lyrics should be allowed to pass under Probably you want this test to be pushed in the same commit where you fix issue 1955. http://codereview.appspot.com/5323062/diff/53040/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): http://codereview.appspot.com/5323062/diff/53040/scm/define-grob-properties.s... scm/define-grob-properties.scm:1040: to this side of the boolean.") .. indicating whether a a span bar is drawn above, or respectively below, this staff.
Sign in to reply to this message.
On Mon, 21 Nov 2011 07:06:53 +0000, k-ohara5a5a@oco.net wrote: > Looks okay to me. > > The ghost of the SpanBar (left over from the issue 910 patch) > sometimes > has an effect > \new GrandStaff << > \new Staff { e'1 | eses'''4 r2. | e'1 } > \new Staff { e'1 | e'1 | eses4 r2. } >> > Something like > \override Score.SpanBar #'extra-spacing-height > = #'(+inf.0 . -inf.0) > takes care of that problem. Fixed. > On 2011/11/10 18:49:48, mike_apollinemike.com wrote: >> On Nov 10, 2011, at 10:38 AM, mailto:k-ohara5a5a@oco.net wrote: > >> > I was talking about custom heights for every bar along the Staff, > >> I think the problem that this'd cause is that the overestimation may > block >> lyrics/dynamics/what-have-you from sliding to the left as much as >> they > could. > > Well, any overestimation in your blocking heights also becomes an > overestimation in the tentative space between Staves/Lyrics/Dynamics > at > the horizontal spacing step. > > For example, the notoriously-bad pure-height estimates for slurs > cause > the tentative-spacing to be looser. Thus a slur (even on a later > line > or later page) will cause Lily to think the Lyrics might have to move > away, and thus avoid the sticky lyrics effect: > \paper {ragged-right = ##t} > \relative c''' { c2 c c c \break c( g,) } > \addlyrics { foo bar foooooooo bar } > > Use of custom locally-determined heights for each bar line seems > unnecessarily complicated (but not as bad as the grace_fixup_list so > I > won't complain). I think that you're right for most cases, but for a piece with lots of accidentals that could potentially hang over barlines, this complexity in estimation seems to help. As for the extra spacing, I'd be curious to see if a gimungous score gets looser with this patch applied. If anyone has a many-page piano or chamber music score with SpanBars, please give it a spin with this patch (and the workaround above that Keith suggests, which will be in my next patch set). > Run fixcc.py on the changed .cc files to clean up some whitespace > problems. Will do. > > > http://codereview.appspot.com/5323062/diff/53040/input/regression/lyrics-pass... > File input/regression/lyrics-pass-under-bar.ly (right): > > > http://codereview.appspot.com/5323062/diff/53040/input/regression/lyrics-pass... > input/regression/lyrics-pass-under-bar.ly:4: texidoc = "Long lyrics > should be allowed to pass under > Probably you want this test to be pushed in the same commit where you > fix issue 1955. You're right. Removed. > > http://codereview.appspot.com/5323062/diff/53040/scm/define-grob-properties.scm > File scm/define-grob-properties.scm (right): > > > http://codereview.appspot.com/5323062/diff/53040/scm/define-grob-properties.s... > scm/define-grob-properties.scm:1040: to this side of the boolean.") > .. indicating whether a a span bar is drawn above, or respectively > below, this staff. Rewritten. Thanks for the feedback! I think we're almost over the hill with this one... Cheers, MS
Sign in to reply to this message.
On Sun, 20 Nov 2011 23:57:22 -0800, <mike@apollinemike.com> wrote: > On Mon, 21 Nov 2011 07:06:53 +0000, k-ohara5a5a@oco.net wrote: > >> Well, any overestimation in your blocking heights also becomes an >> overestimation in the tentative space between Staves/Lyrics/Dynamics >> at the horizontal spacing step. >> > I think that you're right for most cases, but for a piece with lots of > accidentals that could potentially hang over barlines, this complexity > in estimation seems to help. Have an example where pure-from-neighbor would do better than axis-group-interface:height ? > As for the extra spacing, I'd be curious > to see if a gimungous score gets looser with this patch applied. You might have understood me backwards. An overestimation in tentative (vertical) space between Staves/Lyrics/Dynamics will let the spacing spanner do sometimes a /tighter/ horizontal spacing. The overestimated tentative vertical space is forgotten when the real vertical spacing is done. I didn't see, and don't see any risk of, loose spacing from this patch. I expect that occasionally, this patch will let a collision with a span bar leak through, but it would be something crazy like an \espressivo atop a \downbow on a note connected to a cross-staff slur. The \espressivo isn't pure-relevant because of the cross-staff poisoning.
Sign in to reply to this message.
|