Issue 4974075: Add support for custom ledger positions, using two new staff-symbol properties (Closed)
DescriptionAdd support for custom ledger positions, using two new staff-symbol properties
See http://code.google.com/p/lilypond/issues/detail?id=1292 for the proposal.
This also adds support for internal ledger lines, and hence resolves issue http://code.google.com/p/lilypond/issues/detail?id=1193
A regression test for ledger positions is added.
This causes one change in the current regression tests, namely an internal ledger line is added to staff-line-positions.ly. I don't think it is a problem to have internal ledger lines by default.
Patch Set 1 #
Total comments: 8
Patch Set 2 : incorporate joeneemans comments #
Total comments: 12
Patch Set 3 : incorporate Neil Puttock's comments #Patch Set 4 : fixed sigabrt, added/restored several checks #
Total comments: 2
Patch Set 5 : new patch: add checks and warnings for ledger_positions, fix issue #1972 #
Total comments: 3
Patch Set 6 : simpler and more direct check for loop conditions #
Total comments: 1
MessagesTotal messages: 21
Please review my contribution to this wonderful project. Maybe still in time for 2.16 :) On the other hand, it changes quite a bit of the way ledger lines are handled, and I'm not sure what your policy for new features just before release is. Piers
Sign in to reply to this message.
Sign in to reply to this message.
Passes make and reg tests
Sign in to reply to this message.
http://codereview.appspot.com/4974075/diff/1/lily/ledger-line-spanner.cc File lily/ledger-line-spanner.cc (right): http://codereview.appspot.com/4974075/diff/1/lily/ledger-line-spanner.cc#newc... lily/ledger-line-spanner.cc:362: ledgers.add_stencil (ledger_line); Since you no longer seem to be using the brew_ledger_lines function, please get rid of it. http://codereview.appspot.com/4974075/diff/1/lily/staff-symbol.cc File lily/staff-symbol.cc (right): http://codereview.appspot.com/4974075/diff/1/lily/staff-symbol.cc#newcode82 lily/staff-symbol.cc:82: int l = line_positions.size (); Could you use a different name? l looks an awful lot like 1... http://codereview.appspot.com/4974075/diff/1/lily/staff-symbol.cc#newcode105 lily/staff-symbol.cc:105: int l = scm_to_int (scm_length (line_positions)); and again... http://codereview.appspot.com/4974075/diff/1/lily/staff-symbol.cc#newcode117 lily/staff-symbol.cc:117: int l = Staff_symbol::line_count (me); ok, this one isn't your fault, but I still think it would be good to change it.
Sign in to reply to this message.
http://codereview.appspot.com/4974075/diff/1/lily/ledger-line-spanner.cc File lily/ledger-line-spanner.cc (right): http://codereview.appspot.com/4974075/diff/1/lily/ledger-line-spanner.cc#newc... lily/ledger-line-spanner.cc:362: ledgers.add_stencil (ledger_line); On 2011/09/13 21:28:58, joeneeman wrote: > Since you no longer seem to be using the brew_ledger_lines function, please get > rid of it. Done. It had crossed my mind, but I forgot, thanks for noticing. http://codereview.appspot.com/4974075/diff/1/lily/staff-symbol.cc File lily/staff-symbol.cc (right): http://codereview.appspot.com/4974075/diff/1/lily/staff-symbol.cc#newcode82 lily/staff-symbol.cc:82: int l = line_positions.size (); On 2011/09/13 21:28:58, joeneeman wrote: > Could you use a different name? l looks an awful lot like 1... Done. http://codereview.appspot.com/4974075/diff/1/lily/staff-symbol.cc#newcode105 lily/staff-symbol.cc:105: int l = scm_to_int (scm_length (line_positions)); On 2011/09/13 21:28:58, joeneeman wrote: > and again... Done. http://codereview.appspot.com/4974075/diff/1/lily/staff-symbol.cc#newcode117 lily/staff-symbol.cc:117: int l = Staff_symbol::line_count (me); On 2011/09/13 21:28:58, joeneeman wrote: > ok, this one isn't your fault, but I still think it would be good to change it. Done. Indeed, from here I got the l. Much better now.
Sign in to reply to this message.
Passes make and reg tests
Sign in to reply to this message.
http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... File input/regression/staff-ledger-positions.ly (right): http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... input/regression/staff-ledger-positions.ly:4: by setting the @code{ledger-positions} property of the StaffSymbol. wrap around (no space at start of lines) http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... input/regression/staff-ledger-positions.ly:5: The given pattern is repeated. Bracketed groups are always shown together: two spaces follow a full stop http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... input/regression/staff-ledger-positions.ly:11: \version "2.15.11" 2.15.12 http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... input/regression/staff-ledger-positions.ly:14: \new Staff \relative c' { c' { http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... input/regression/staff-ledger-positions.ly:15: \override Staff.StaffSymbol #'line-positions = #'( -5 -2 -1 2 5 6 ) (-5 -2 -1 2 5 6) http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... input/regression/staff-ledger-positions.ly:16: \override Staff.StaffSymbol #'ledger-positions = #'( -5 (-2 -1) 2 ) (-5 (-2 -1) 2) http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... input/regression/staff-ledger-positions.ly:18: g, c e b' c'' e g g,4 http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... input/regression/staff-ledger-positions.ly:20: remove extra newlines http://codereview.appspot.com/4974075/diff/8001/lily/staff-symbol.cc File lily/staff-symbol.cc (right): http://codereview.appspot.com/4974075/diff/8001/lily/staff-symbol.cc#newcode105 lily/staff-symbol.cc:105: int line_count = scm_to_int (scm_length (line_positions)); scm_ilength (line_positions) http://codereview.appspot.com/4974075/diff/8001/lily/staff-symbol.cc#newcode243 lily/staff-symbol.cc:243: return scm_to_int (scm_length (line_positions)); scm_ilength (line_positions) http://codereview.appspot.com/4974075/diff/8001/scm/define-grob-properties.scm File scm/define-grob-properties.scm (right): http://codereview.appspot.com/4974075/diff/8001/scm/define-grob-properties.sc... scm/define-grob-properties.scm:501: (ledger-extra ,number? "Extra distance from staff line to draw ledger ,ly:dimension? http://codereview.appspot.com/4974075/diff/8001/scm/define-grob-properties.sc... scm/define-grob-properties.scm:508: of ledger lines. Bracketed groups are always shown together.") two spaces after `lines.'
Sign in to reply to this message.
On 2011/09/14 22:12:22, Neil Puttock wrote: > http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... > File input/regression/staff-ledger-positions.ly (right): > > http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... > input/regression/staff-ledger-positions.ly:4: by setting the > @code{ledger-positions} property of the StaffSymbol. > wrap around (no space at start of lines) > > http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... > input/regression/staff-ledger-positions.ly:5: The given pattern is repeated. > Bracketed groups are always shown together: > two spaces follow a full stop > > http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... > input/regression/staff-ledger-positions.ly:11: \version "2.15.11" > 2.15.12 > > http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... > input/regression/staff-ledger-positions.ly:14: \new Staff \relative c' { > c' { > > http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... > input/regression/staff-ledger-positions.ly:15: \override Staff.StaffSymbol > #'line-positions = #'( -5 -2 -1 2 5 6 ) > (-5 -2 -1 2 5 6) > > http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... > input/regression/staff-ledger-positions.ly:16: \override Staff.StaffSymbol > #'ledger-positions = #'( -5 (-2 -1) 2 ) > (-5 (-2 -1) 2) > > http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... > input/regression/staff-ledger-positions.ly:18: g, c e b' c'' e g > g,4 > > http://codereview.appspot.com/4974075/diff/8001/input/regression/staff-ledger... > input/regression/staff-ledger-positions.ly:20: > remove extra newlines > > http://codereview.appspot.com/4974075/diff/8001/lily/staff-symbol.cc > File lily/staff-symbol.cc (right): > > http://codereview.appspot.com/4974075/diff/8001/lily/staff-symbol.cc#newcode105 > lily/staff-symbol.cc:105: int line_count = scm_to_int (scm_length > (line_positions)); > scm_ilength (line_positions) > > http://codereview.appspot.com/4974075/diff/8001/lily/staff-symbol.cc#newcode243 > lily/staff-symbol.cc:243: return scm_to_int (scm_length (line_positions)); > scm_ilength (line_positions) > > http://codereview.appspot.com/4974075/diff/8001/scm/define-grob-properties.scm > File scm/define-grob-properties.scm (right): > > http://codereview.appspot.com/4974075/diff/8001/scm/define-grob-properties.sc... > scm/define-grob-properties.scm:501: (ledger-extra ,number? "Extra distance from > staff line to draw ledger > ,ly:dimension? > > http://codereview.appspot.com/4974075/diff/8001/scm/define-grob-properties.sc... > scm/define-grob-properties.scm:508: of ledger lines. Bracketed groups are always > shown together.") > two spaces after `lines.' Done, typical beginner imperfections, thanks for pointing out.
Sign in to reply to this message.
On 15 September 2011 14:43, <pierstitus@gmail.com> wrote: > Done, typical beginner imperfections, thanks for pointing out. Thanks. :) For some reason, your patch fails `make check' on my system. I consistently get SIGABRT thrown soon after running: job 2 terminated with signal: 6 job 1 terminated with signal: 6 job 0 terminated with signal: 6 fatal error: Children (2 1 0) exited with errors. command failed: /home/neil/lilypond/out/bin/lilypond -I ./ -I ./out-test -I ../../input -I /home/neil/lilypond/Documentation -I /home/neil/lilypond/Documentation/snippets -I ../../input/regression/ -I /home/neil/lilypond/Documentation/included/ -I /home/neil/lilypond/mf/out/ -I /home/neil/lilypond/mf/out/ -I /home/neil/lilypond/Documentation/pictures -I /home/neil/lilypond/Documentation/pictures/./out-test -dbackend=eps --formats=ps -djob-count=3 -dseparate-log-files -dinclude-eps-fonts -dgs-load-lily-fonts --header=texidoc -I /home/neil/lilypond/Documentation/included/ -ddump-profile -dcheck-internal-types -ddump-signatures -danti-alias-factor=1 -I "/home/neil/lilypond/out/lybook-testdb" -I "/home/neil/lilypond/input/regression" -I "/home/neil/lilypond/input/regression" -I "/home/neil/lilypond/input/regression/out-test" -I "/home/neil/lilypond/input" -I "/home/neil/lilypond/Documentation" -I "/home/neil/lilypond/Documentation/snippets" -I "/home/neil/lilypond/input/regression" -I "/home/neil/lilypond/Documentation/included" -I "/home/neil/lilypond/mf/out" -I "/home/neil/lilypond/mf/out" -I "/home/neil/lilypond/Documentation/pictures" -I "/home/neil/lilypond/Documentation/pictures/out-test" --formats=eps -deps-box-padding=3.000000 -dread-file-list -dno-strip-output-dir "/home/neil/lilypond/out/lybook-testdb/snippet-names--7968346798296354682.ly" Child returned 1 make[2]: *** [out-test/collated-files.texi] Error 1 rm out-test/weblinks.itexi make[2]: Leaving directory `/home/neil/lilypond/input/regression' make[1]: *** [local-test] Error 2 make[1]: Leaving directory `/home/neil/lilypond/input/regression' make: *** [test] Error 2 This is with an unoptimised binary (using --disable-optimising). Cheers, Neil
Sign in to reply to this message.
On 2011/09/15 21:41:27, Neil Puttock wrote: > For some reason, your patch fails `make check' on my system. Neil, I've just applied this patch - after seeing your note - to the latest 'git pull -r' and 'make ; make check' work fine on my system if that helps? James
Sign in to reply to this message.
On 15 September 2011 23:04, <pkx166h@gmail.com> wrote: > On 2011/09/15 21:41:27, Neil Puttock wrote: > >> For some reason, your patch fails `make check' on my system. > > Neil, I've just applied this patch - after seeing your note - to the > latest 'git pull -r' and 'make ; make check' work fine on my system if > that helps? Did you build with --disable-optimising (you should be if you're doing regression checking)? Cheers, Neil
Sign in to reply to this message.
On 2011/09/15 22:05:51, Neil Puttock wrote: > On 15 September 2011 23:04, <mailto:pkx166h@gmail.com> wrote: > > On 2011/09/15 21:41:27, Neil Puttock wrote: > > > >> For some reason, your patch fails `make check' on my system. > > > > Neil, I've just applied this patch - after seeing your note - to the > > latest 'git pull -r' and 'make ; make check' work fine on my system if > > that helps? > > Did you build with --disable-optimising (you should be if you're doing > regression checking)? > > Cheers, > Neil Ah, I didn't know --disable-optimising enables the assertions, very useful. I confirm the SIGABRT, and will look further into it. It seems I have to do a bit more clean up. Also for documentation I need to add a snippet, isn't it?
Sign in to reply to this message.
regtests now build with --disable-optimising. Everything should be OK by now.
Sign in to reply to this message.
Passes make and reg tests
Sign in to reply to this message.
Hi, i see that i'm the owner of the tracker issue 1193 (and i suppose that Piers doesn't have git push ability), so i guess that it's for me to push. However, i don't understand some things: - what exactly shall i push? - is this Rietveld issue up-to-date? I see that it was last modified week ago, while the discussion at the tracker is more recent (for example i see staff-ledger-internal.ly attached to http://code.google.com/p/lilypond/issues/detail?id=1193#c23 , but it's not here) - if the patch was indeed modified after the countdown, should it have another countdown before pushing? cheers, Janek
Sign in to reply to this message.
Piers, there are segfaults and infinite loops reported at http://code.google.com/p/lilypond/issues/detail?id=1972 http://codereview.appspot.com/4974075/diff/21001/lily/staff-symbol.cc File lily/staff-symbol.cc (right): http://codereview.appspot.com/4974075/diff/21001/lily/staff-symbol.cc#newcode186 lily/staff-symbol.cc:186: SCM s = scm_cdr (ledger_positions); if ledger-positions = '(1) we probably shouldn't enter the do loop; line 218 might segfault http://codereview.appspot.com/4974075/diff/21001/lily/staff-symbol.cc#newcode192 lily/staff-symbol.cc:192: current = scm_to_double (s2) + n * cycle; if ledger-positions are equal, 0.0==cycle, so current does not advance
Sign in to reply to this message.
On 2011/10/17 05:11:59, Keith wrote: > Piers, there are segfaults and infinite loops reported at > http://code.google.com/p/lilypond/issues/detail?id=1972 I uploaded a new patch that fixes this issue, and gives a warning about unexpected behaviour when the list is not sorted. When the list is containing only 1 value or only the same values a warning is given and the file is compiled without ledger lines. Is a warning OK, or should that be an error?
Sign in to reply to this message.
Most of the complexity is due to the repeating cycle facility. First, you might try out that feature, and reconsider whether it is worth the complexity in the code. http://codereview.appspot.com/4974075/diff/28001/lily/staff-symbol.cc File lily/staff-symbol.cc (right): http://codereview.appspot.com/4974075/diff/28001/lily/staff-symbol.cc#newcode184 lily/staff-symbol.cc:184: me->warning (_ ("ledger_positions is not sorted, may show unexpected behaviour")); Probably not necessary, because the printed ledger lines demonstrate the behavior. If somebody re-arranges the ledger lines, does the warning print once per system or just once when they are set? http://codereview.appspot.com/4974075/diff/28001/lily/staff-symbol.cc#newcode186 lily/staff-symbol.cc:186: if (cycle < 0.1) cycle = 2.0; http://codereview.appspot.com/4974075/diff/28001/lily/staff-symbol.cc#newcode200 lily/staff-symbol.cc:200: do We want a test here to ensure we can follow the tail of 's' in line 231 below. Otherwise a mysterious segfault could appear if the test above involving 'cycle' is ever changed.
Sign in to reply to this message.
New, simpler, patch added On 2011/10/17 18:56:58, Keith wrote: > Most of the complexity is due to the repeating cycle facility. First, you might > try out that feature, and reconsider whether it is worth the complexity in the > code. I tried it for a number of notations from http://musicnotation.org/ Otherwise it should be specified with repetitions, enough to cover a sensible range, but now it is more universal. > > http://codereview.appspot.com/4974075/diff/28001/lily/staff-symbol.cc > File lily/staff-symbol.cc (right): > > http://codereview.appspot.com/4974075/diff/28001/lily/staff-symbol.cc#newcode184 > lily/staff-symbol.cc:184: me->warning (_ ("ledger_positions is not sorted, may > show unexpected behaviour")); > Probably not necessary, because the printed ledger lines demonstrate the > behavior. If somebody re-arranges the ledger lines, does the warning print once > per system or just once when they are set? indeed it prints the warning once per system, not nice. > > http://codereview.appspot.com/4974075/diff/28001/lily/staff-symbol.cc#newcode186 > lily/staff-symbol.cc:186: if (cycle < 0.1) > cycle = 2.0; also possible, but if input is incorrect I'd prefer to not print ledger lines at all, that makes it more clear something is wrong. > > http://codereview.appspot.com/4974075/diff/28001/lily/staff-symbol.cc#newcode200 > lily/staff-symbol.cc:200: do > We want a test here to ensure we can follow the tail of 's' in line 231 below. > Otherwise a mysterious segfault could appear if the test above involving 'cycle' > is ever changed. I made the patch much simpler, it just checks the conditions that can go wrong in the loop, and doesn't print ledgers if those conditions are not met. Is this what you mean? Alternative I could rewrite the loops as for loops, which might be more clear, but I don't think it's worth the effort, especially since for loops such as on line 166 are not clear as water either. Piers
Sign in to reply to this message.
One obvious change to get it to compile, and then it looks good to me. If you email me a patch from `git format-patch` I'll push it with you as author; otherwise I'll just push it after countdown. http://codereview.appspot.com/4974075/diff/33001/lily/staff-symbol.cc File lily/staff-symbol.cc (right): http://codereview.appspot.com/4974075/diff/33001/lily/staff-symbol.cc#newcode188 lily/staff-symbol.cc:188: if (!is_pair(s) || cycle < 0.1) if (!scm_is_pair (s) || cycle < 0.1) You can run `scripts/auxiliar/fixcc.py foo.cc` to put in the GNU-coding-style spaces.
Sign in to reply to this message.
|
