Issue 1320: Scheme bar line interface

Issue 1320: Scheme bar line interface

This patch moves most of the code from
lily/bar-line.cc and lily/span-bar.cc into scm/bar-line.scm

After this patch is applied, we will move to the new bar line
input handling routines, but it is easier for the revisiting
process to split the work in two parts.

[MikeSol, 2012-06-20 19:13:01]

Good work - two overall things.

1) In Scheme, try to avoid `do'.  Use map and reduce.
2) Have you tested performance hits on large scores?  It may be worth it to
leave bar-line.cc as the default and have your script as a Scheme
implementation, not unlike the way we do bezier curves.

Cheers,
MS

http://codereview.appspot.com/6305115/diff/1/lily/bar-line.cc
File lily/bar-line.cc (right):

http://codereview.appspot.com/6305115/diff/1/lily/bar-line.cc#newcode31
lily/bar-line.cc:31: #include <set>
Is it worth it to keep this file?  I'd just move what's left over to Scheme at
that point.

http://codereview.appspot.com/6305115/diff/1/lily/span-bar.cc
File lily/span-bar.cc (right):

http://codereview.appspot.com/6305115/diff/1/lily/span-bar.cc#newcode36
lily/span-bar.cc:36: Pointer_group_interface::add_grob (me, ly_symbol2scm
("elements"), b);
Ditto - you can likely get rid of this as well.

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm
File scm/bar-line.scm (right):

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode80
scm/bar-line.scm:80: (cons (- height half-staff) (+ height half-staff))
(interval-widen half-staff height)

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode94
scm/bar-line.scm:94: (stencil empty-stencil))
As you use let*, you can move all the set! to redefinitions of stencil in the
let*. It seems slightly more in keeping w/ coding practice (could be wrong,
though).

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode118
scm/bar-line.scm:118: 
To be more scheme-ic, try a map to get an array of stencils w/ the correct
offsets and then using reduce on the list of stencils to smoosh them together
via ly:stencil-add.

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode150
scm/bar-line.scm:150: stencil)
Ditto - as a general rule, avoid `do'. Map and reduce.

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode214
scm/bar-line.scm:214: X RIGHT colon-stil kern))
Can you turn the above into the result of a call to a recursive function that
uses ly:stencil-combine-at-edge?

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode495
scm/bar-line.scm:495: ;; bulky side. Rewritten by Han-Wen. Ported from c++ to
Scheme by Marc Hohl.
Can you get this down to the 80 character line width?

[benko.pal, 2012-06-20 19:34:58]

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm
File scm/bar-line.scm (right):

http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode83
scm/bar-line.scm:83: (define (make-colon-bar-line grob)
I'm afraid this defun doesn't match the relevant part of current
Bar_line::compound_barline.

try

\new Staff {
  \override Staff.StaffSymbol #'line-positions = #'(-2 0 2 4)
  s1 \bar ":|"
}

[marc, 2012-06-21 06:32:39]

Am 20.06.2012 21:13, schrieb mtsolo@gmail.com:
> Good work - two overall things.
>
> 1) In Scheme, try to avoid `do'. Use map and reduce.
Well, I am not very familiar with scheme, and moreover, the whole
stencil combination stuff will be replaced anyway in the second step – see
http://lists.gnu.org/archive/html/lilypond-devel/2012-06/msg00066.html

> 2) Have you tested performance hits on large scores? It may be worth it
> to leave bar-line.cc as the default and have your script as a Scheme
> implementation, not unlike the way we do bezier curves.
No, not yet.
>
> Cheers,
> MS
>
>
> http://codereview.appspot.com/6305115/diff/1/lily/bar-line.cc
> File lily/bar-line.cc (right):
>
> http://codereview.appspot.com/6305115/diff/1/lily/bar-line.cc#newcode31
> lily/bar-line.cc:31: #include <set>
> Is it worth it to keep this file? I'd just move what's left over to
> Scheme at that point.
As far as I see, Bar_line::non_empty_barline is used
in lily/note-spacing.cc and lily/paper-column.cc.
I don't know how to redefine this in Scheme and make it
available in these .cc files.
>
> http://codereview.appspot.com/6305115/diff/1/lily/span-bar.cc
> File lily/span-bar.cc (right):
>
> http://codereview.appspot.com/6305115/diff/1/lily/span-bar.cc#newcode36
> lily/span-bar.cc:36: Pointer_group_interface::add_grob (me,
> ly_symbol2scm ("elements"), b);
> Ditto - you can likely get rid of this as well.
Same as above.
>
> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm
> File scm/bar-line.scm (right):
>
> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode80
> scm/bar-line.scm:80: (cons (- height half-staff) (+ height half-staff))
> (interval-widen half-staff height)
The definition of interval-widen requires the first argument to
be an interval, which half-staff isn't :-(
>
> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode94
> scm/bar-line.scm:94: (stencil empty-stencil))
> As you use let*, you can move all the set! to redefinitions of stencil
> in the let*. It seems slightly more in keeping w/ coding practice (could
> be wrong, though).
Thanks, done.
>
> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode118
> scm/bar-line.scm:118:
> To be more scheme-ic, try a map to get an array of stencils w/ the
> correct offsets and then using reduce on the list of stencils to smoosh
> them together via ly:stencil-add.
Uh? I'll give it a try later, when more urgent problems are solved.
>
> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode150
> scm/bar-line.scm:150: stencil)
> Ditto - as a general rule, avoid `do'. Map and reduce.
>
> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode214
> scm/bar-line.scm:214: X RIGHT colon-stil kern))
> Can you turn the above into the result of a call to a recursive function
> that uses ly:stencil-combine-at-edge?
As said above: the whole stencil combination stuff is meant to
be replaced completely. The main idea here is to translate c++ into
scheme mostly 1:1 for better comparison.
>
> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode495
> scm/bar-line.scm:495: ;; bulky side. Rewritten by Han-Wen. Ported from
> c++ to Scheme by Marc Hohl.
> Can you get this down to the 80 character line width?
Done.
>
> http://codereview.appspot.com/6305115/
>

[marc, 2012-06-21 06:35:55]

Am 20.06.2012 21:34, schrieb benko.pal@gmail.com:
>
> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm
> File scm/bar-line.scm (right):
>
> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode83
> scm/bar-line.scm:83: (define (make-colon-bar-line grob)
> I'm afraid this defun doesn't match the relevant part of current
> Bar_line::compound_barline.
>
> try
>
> \new Staff {
> \override Staff.StaffSymbol #'line-positions = #'(-2 0 2 4)
> s1 \bar ":|"
> }
Thanks for the hint! I didn't check the results with altered line 
positions yet,
but I don't understand what's wrong here – IMHO the scheme stuff mimics
1:1 the code from the original definition.

What is wrong here?
>
> http://codereview.appspot.com/6305115/
>

[marc, 2012-06-21 07:54:16]

Am 20.06.2012 21:13, schrieb mtsolo@gmail.com:
> Good work - two overall things.
>
> [...]
> 2) Have you tested performance hits on large scores?  It may be worth it
> to leave bar-line.cc as the default and have your script as a Scheme
> implementation, not unlike the way we do bezier curves.
>
I did a test with a considerably large file, first with the current master:
real    0m36.592s
user    0m31.902s
sys    0m1.216s

then with the new patch:
real    0m32.166s
user    0m31.118s
sys    0m0.992s

and then again with master (I think lilypond does some caching, no?):

real    0m32.065s
user    0m31.046s
sys    0m1.012s

So if I didn't do anything completely wrong, there is no noticeable 
difference
between letting Lily do the barlines the c++ way or scheme-ishly.

Regards,

Marc

[marc, 2012-06-21 09:05:47]

Am 21.06.2012 08:35, schrieb Marc Hohl:
> Am 20.06.2012 21:34, schrieb benko.pal@gmail.com:
>>
>> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm
>> File scm/bar-line.scm (right):
>>
>> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode83
>> scm/bar-line.scm:83: (define (make-colon-bar-line grob)
>> I'm afraid this defun doesn't match the relevant part of current
>> Bar_line::compound_barline.
>>
>> try
>>
>> \new Staff {
>> \override Staff.StaffSymbol #'line-positions = #'(-2 0 2 4)
>> s1 \bar ":|"
>> }
> Thanks for the hint! I didn't check the results with altered line 
> positions yet,
> but I don't understand what's wrong here – IMHO the scheme stuff mimics
> 1:1 the code from the original definition.
>
> What is wrong here?
After some research, I found of that line-count is wrong,
I get a value of 5 instead of 4!

So something along

  (let* ((staff-symbol (ly:grob-object grob 'staff-symbol))
          (line-count (if (ly:grob? staff-symbol)
                          (ly:grob-property staff-symbol 'line-count)
                          0))
...

is fishy
>>
>> http://codereview.appspot.com/6305115/
>>
>
>

[benko.pal, 2012-06-23 09:06:41]

hi Marc,

>> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm
>> File scm/bar-line.scm (right):
>>
>> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode83
>> scm/bar-line.scm:83: (define (make-colon-bar-line grob)
>> I'm afraid this defun doesn't match the relevant part of current
>> Bar_line::compound_barline.
>>
>> try
>>
>> \new Staff {
>> \override Staff.StaffSymbol #'line-positions = #'(-2 0 2 4)
>> s1 \bar ":|"
>> }
>
> Thanks for the hint! I didn't check the results with altered line positions
> yet,
> but I don't understand what's wrong here – IMHO the scheme stuff mimics
> 1:1 the code from the original definition.

that definition was changed considerably in the meantime.

> What is wrong here?

in other threads this was answered, but let me summarize from another POV
(noting that I'm not at all an expert):
we have several conflicting properties (line-count, line-positions,
staff-space),
used in c++ generally through accessors; those accessors grew
more sophisticated and mean no more the same as the property (as you saw
line-count of scheme vs. line_count of c++).
we even have some abstractions built on them (e.g. staff_radius), and that
just adds to the confusion, which could grow for quite some time because
noone uses these fancy staffs.  well, I need them from time to time, so lately
my work with Lilypond went to straightening up some of these issues.

unfortunately I have just a little time for Lilypond and I'm much better versed
in c++ than scheme, so I can't really help you with this project.

p

[marc, 2012-06-23 09:11:59]

Am 23.06.2012 11:06, schrieb Benkő Pál:
> hi Marc,
>
>>> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm
>>> File scm/bar-line.scm (right):
>>>
>>> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode83
>>> scm/bar-line.scm:83: (define (make-colon-bar-line grob)
>>> I'm afraid this defun doesn't match the relevant part of current
>>> Bar_line::compound_barline.
>>>
>>> try
>>>
>>> \new Staff {
>>> \override Staff.StaffSymbol #'line-positions = #'(-2 0 2 4)
>>> s1 \bar ":|"
>>> }
>> Thanks for the hint! I didn't check the results with altered line positions
>> yet,
>> but I don't understand what's wrong here – IMHO the scheme stuff mimics
>> 1:1 the code from the original definition.
> that definition was changed considerably in the meantime.
I just noticed that ... sorry for that!
>
>> What is wrong here?
> in other threads this was answered, but let me summarize from another POV
> (noting that I'm not at all an expert):
> we have several conflicting properties (line-count, line-positions,
> staff-space),
> used in c++ generally through accessors; those accessors grew
> more sophisticated and mean no more the same as the property (as you saw
> line-count of scheme vs. line_count of c++).
Yes.
> we even have some abstractions built on them (e.g. staff_radius), and that
> just adds to the confusion, which could grow for quite some time because
> noone uses these fancy staffs.  well, I need them from time to time, so
lately
> my work with Lilypond went to straightening up some of these issues.
Thanks for clarification! I'll try to implement the changes for the
colon stencil within the next few days...
>
> unfortunately I have just a little time for Lilypond and I'm much better
versed
> in c++ than scheme, so I can't really help you with this project.
Never mind – checking my current work against your fancy staffs is
very helpful for me!

Regards,

Marc
>
> p
>

[marc, 2012-06-26 19:28:51]

Am 23.06.2012 11:11, schrieb Marc Hohl:
> Am 23.06.2012 11:06, schrieb Benkő Pál:
>> hi Marc,
>>
>>>> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm
>>>> File scm/bar-line.scm (right):
>>>>
>>>> http://codereview.appspot.com/6305115/diff/1/scm/bar-line.scm#newcode83 
>>>>
>>>> scm/bar-line.scm:83: (define (make-colon-bar-line grob)
>>>> I'm afraid this defun doesn't match the relevant part of current
>>>> Bar_line::compound_barline.
>>>>
>>>> try
>>>>
>>>> \new Staff {
>>>> \override Staff.StaffSymbol #'line-positions = #'(-2 0 2 4)
>>>> s1 \bar ":|"
>>>> }
>>> Thanks for the hint! I didn't check the results with altered line 
>>> positions
>>> yet,
>>> but I don't understand what's wrong here – IMHO the scheme stuff mimics
>>> 1:1 the code from the original definition.
>> that definition was changed considerably in the meantime.
> I just noticed that ... sorry for that!
OK, so I'm either blind or stupid or whatever.

I tried to adapt Benkő Páls algorithm for the colon stencil; I think the 
scheme version
calculates all values correctly, but the value for dist and center are 
resetted after
the calculation process.
You get dist = 1.0 at the beginning, dist = 2.0 in the loop and dist = 
1.0 afterwards.
After over an hour sitting in front of my computer without finding the 
error I'm
somewhat frustrated. I think I'll dream in indentations and brackets 
tonight :(

Marc

[Keith, 2012-07-16 22:58:04]

Wherever there is an empty bar-line, \bar"" , this patch reserves too much space
for the bar-line.  Possibly the extent of empty bar-lines is computed wrongly.

[marc, 2012-07-17 07:43:23]

Am 17.07.2012 00:58, schrieb k-ohara5a5a@oco.net:
> Wherever there is an empty bar-line, \bar"" , this patch reserves too
> much space for the bar-line.  Possibly the extent of empty bar-lines is
> computed wrongly.
Obviously, I swapped the x- and y-coordinates within the definition
of make-empty-barline.

Will upload the corrections after the tests have finished.

Thanks for spotting this!

Marc
>
> http://codereview.appspot.com/6305115/
>

[marc, 2012-07-17 19:48:39]

The regression test

span-par-allow-span-bar.ly

works now as expected. I added some comments explaining
how the information is used/modified.

[marc, 2012-07-20 10:15:41]

Am 17.07.2012 21:48, schrieb marc@hohlart.de:
> The regression test
>
> span-par-allow-span-bar.ly
Should read

span-bar-allow-span-bar.ly

;-)

After

make test-redo; make check

I still see some small differences between master and my patch.

I think the differences concerning MIDI output can safely be
ignored, but a lot of spacing issues remain. I don't understand
where they come from; because *a lot of* files are concerned, I
suspected the simple bar line to be the culprit, but IMHO the
definition of make-simple-bar-line mimics 1:1 the definition in
lily/bar-line.cc: Bar_line::simple_barline.

Regards,

Marc


>
> works now as expected. I added some comments explaining
> how the information is used/modified.
>
> http://codereview.appspot.com/6305115/
>
> _______________________________________________
> lilypond-devel mailing list
> lilypond-devel@gnu.org
> https://lists.gnu.org/mailman/listinfo/lilypond-devel
>

[dak, 2012-07-20 10:24:08]

On 2012/07/20 10:15:41, marc wrote:

> I think the differences concerning MIDI output can safely be
> ignored,

What makes it safe to ignore them?

[mail_philholmes.net, 2012-07-20 10:26:16]

----- Original Message ----- 
From: "Marc Hohl" <marc@hohlart.de>
To: <marc@hohlart.de>; <mtsolo@gmail.com>; <benko.pal@gmail.com>; 
<k-ohara5a5a@oco.net>; <lilypond-devel@gnu.org>; 
<reply@codereview-hr.appspotmail.com>
Sent: Friday, July 20, 2012 11:15 AM
Subject: Re: Issue 1320: Scheme bar line interface (issue 6305115)


> Am 17.07.2012 21:48, schrieb marc@hohlart.de:
>> The regression test
>>
>> span-par-allow-span-bar.ly
> Should read
>
> span-bar-allow-span-bar.ly
>
> ;-)
>
> After
>
> make test-redo; make check
>
> I still see some small differences between master and my patch.
>
> I think the differences concerning MIDI output can safely be
> ignored, but a lot of spacing issues remain. I don't understand
> where they come from; because *a lot of* files are concerned, I
> suspected the simple bar line to be the culprit, but IMHO the
> definition of make-simple-bar-line mimics 1:1 the definition in
> lily/bar-line.cc: Bar_line::simple_barline.
>
> Regards,
>
> Marc
>
>
>>
>> works now as expected. I added some comments explaining
>> how the information is used/modified.

It might be worth your comparing the differing output by eye - the regtest 
checker only gives a rough idea of where the differences lie.  Failing that, 
I think I could create a windows .exe if the patch was in an accessible 
branch on git, and could run my regtest pixel comparator.  That would take a 
while, though, so I' prefer only to do it if all else failed.

--
Phil Holmes 

[marc, 2012-07-20 18:17:07]

Am 20.07.2012 12:24, schrieb dak@gnu.org:
> On 2012/07/20 10:15:41, marc wrote:
>
>> I think the differences concerning MIDI output can safely be
>> ignored,
>
> What makes it safe to ignore them?
Well, I don't know.

But diffs like

@@ -1,4 +1,5 @@
  Parsing...
+Renaming input to: `out-test/voice-5-midi.ly'
  Renaming input to: `out-test/voice-5-midi.ly'
  Interpreting music...
  Preprocessing graphical objects...
@@ -14,3 +15,5 @@
  Writing ./b7/lily-6a4f9eea-systems.tex...
  Writing ./b7/lily-6a4f9eea-systems.count...
  Writing timing to b7/lily-6a4f9eea.profile...
+Writing timing to ./lily-run-total.profile...
+Success: compilation successfully completed

look like internal changes during the compilation to me.
I cannot imagine how a scheme-created stencil would influence the
MIDI output in any way.

I wonder what Patchy would say about this, but it seems that my current changes
are not taken into account by him/her(?) yet.

Regards,

Marc


>
> http://codereview.appspot.com/6305115/
>

[marc, 2012-07-20 18:22:22]

Am 20.07.2012 12:26, schrieb Phil Holmes:
[...]
> It might be worth your comparing the differing output by eye - the 
> regtest checker only gives a rough idea of where the differences lie.  
> Failing that, I think I could create a windows .exe if the patch was 
> in an accessible branch on git, and could run my regtest pixel 
> comparator.  That would take a while, though, so I' prefer only to do 
> it if all else failed.
Looking at the differences in out/test-results/index.html, I see some 
blur around notes
or bar lines, but the differences seem to be very small.

A click on 'details' shows a lot of entries like

Stem: 0.056319, Stem: 0.056319, Stem: 0.056319, Stem: 0.056319, BarLine: 
0.050704, ...

I don't know if this is acceptable, and still I do not understand how these
differences can occur from the scheme code ...

Regards,

Marc

[dak, 2012-07-20 18:28:34]

Marc Hohl <marc@hohlart.de> writes:

> Am 20.07.2012 12:24, schrieb dak@gnu.org:
>> On 2012/07/20 10:15:41, marc wrote:
>>
>>> I think the differences concerning MIDI output can safely be
>>> ignored,
>>
>> What makes it safe to ignore them?
> Well, I don't know.
>
> But diffs like
>
> @@ -1,4 +1,5 @@
>  Parsing...
> +Renaming input to: `out-test/voice-5-midi.ly'
>  Renaming input to: `out-test/voice-5-midi.ly'
>  Interpreting music...
>  Preprocessing graphical objects...
> @@ -14,3 +15,5 @@
>  Writing ./b7/lily-6a4f9eea-systems.tex...
>  Writing ./b7/lily-6a4f9eea-systems.count...
>  Writing timing to b7/lily-6a4f9eea.profile...
> +Writing timing to ./lily-run-total.profile...
> +Success: compilation successfully completed
>
> look like internal changes during the compilation to me.

Sure.

> I cannot imagine how a scheme-created stencil would influence the
> MIDI output in any way.

Well, _here_ you use "MIDI output" meaning the output in MIDI format,
not console messages during MIDI-related regtests.  But when you stated
"I think the differences concerning MIDI output can safely be ignored,"
you used it in an unusual meaning.


-- 
David Kastrup

[marc, 2012-07-20 18:31:19]

Am 20.07.2012 20:17, schrieb Marc Hohl:
> Am 20.07.2012 12:24, schrieb dak@gnu.org:
>> On 2012/07/20 10:15:41, marc wrote:
>>
>>> I think the differences concerning MIDI output can safely be
>>> ignored,
Thinko – IIUC, not the MIDI output is changed, only the log file.

Regards,
Marc

http://codereview.appspot.com/6305115/

[marc, 2012-07-20 18:44:00]

Am 20.07.2012 20:28, schrieb David Kastrup:
> Marc Hohl <marc@hohlart.de> writes:
>
>> Am 20.07.2012 12:24, schrieb dak@gnu.org:
>>> On 2012/07/20 10:15:41, marc wrote:
>>>
>>>> I think the differences concerning MIDI output can safely be
>>>> ignored,
>>> What makes it safe to ignore them?
>> Well, I don't know.
>>
>> But diffs like
>>
>> @@ -1,4 +1,5 @@
>>   Parsing...
>> +Renaming input to: `out-test/voice-5-midi.ly'
>>   Renaming input to: `out-test/voice-5-midi.ly'
>>   Interpreting music...
>>   Preprocessing graphical objects...
>> @@ -14,3 +15,5 @@
>>   Writing ./b7/lily-6a4f9eea-systems.tex...
>>   Writing ./b7/lily-6a4f9eea-systems.count...
>>   Writing timing to b7/lily-6a4f9eea.profile...
>> +Writing timing to ./lily-run-total.profile...
>> +Success: compilation successfully completed
>>
>> look like internal changes during the compilation to me.
> Sure.
>
>> I cannot imagine how a scheme-created stencil would influence the
>> MIDI output in any way.
> Well, _here_ you use "MIDI output" meaning the output in MIDI format,
> not console messages during MIDI-related regtests.  But when you stated
> "I think the differences concerning MIDI output can safely be ignored,"
> you used it in an unusual meaning.
>
>
Yes, sorry for being imprecise – it is only the console messages that 
differ.

Regards,

Marc

[dak, 2012-07-20 19:03:20]

http://codereview.appspot.com/6305115/diff/30001/scm/bar-line.scm
File scm/bar-line.scm (right):

http://codereview.appspot.com/6305115/diff/30001/scm/bar-line.scm#newcode649
scm/bar-line.scm:649: 
Empty line at end of file makes git complain about whitespace error.

[benko.pal, 2012-07-20 19:15:14]

Marc, please don't throw the whole 2533 issue stuff out;
look at the newer version at
http://codereview.appspot.com/6431044

p

[Keith, 2012-07-20 21:16:01]

Works fine for me, but it will need updated to synchronize with the patches
soon-to-be-pushed to the C version.

Since the C code is getting bug-fixes and improvements at the same time as you
port to Scheme, you should incorporate the improved behavior into your Scheme
code as soon as you agree that it is correct.  When the changed C code is
pushed, you will need to rebase your patch, but the state of code we are
reviewing will not change so often.

Marks and bar-numbers above repeat bars now seem to align to the center of the
repeat bar including the dots, where before they aligned to just the double bar
portion. Probably the reference point of the stencil has changed.

The other regtest changes look acceptable to me, and it seems to work
essentially as fast as the C code on real music.

http://codereview.appspot.com/6305115/diff/30001/lily/bar-line.cc
File lily/bar-line.cc (left):

http://codereview.appspot.com/6305115/diff/30001/lily/bar-line.cc#oldcode66
lily/bar-line.cc:66: Bar_line::non_empty_barline (Grob *me)
Good to keep this in C.  It used to live in paper-column.cc until the fix to
issue 915.  Later, if we want to remove the file completely, it could go back
there, later.

http://codereview.appspot.com/6305115/diff/30001/lily/bar-line.cc#oldcode426
lily/bar-line.cc:426: ADD_INTERFACE (Bar_line,
Would this make more sense in define-grob-interfaces.scm ?
It would become the largest docstring in that file, but I don't see any problem
with that.

http://codereview.appspot.com/6305115/diff/30001/lily/grob-scheme.cc
File lily/grob-scheme.cc (right):

http://codereview.appspot.com/6305115/diff/30001/lily/grob-scheme.cc#newcode459
lily/grob-scheme.cc:459: "Compare two grobs.")
"Does @var{a} lie above @var{b} on the page?"

http://codereview.appspot.com/6305115/diff/30001/scm/bar-line.scm
File scm/bar-line.scm (right):

http://codereview.appspot.com/6305115/diff/30001/scm/bar-line.scm#newcode67
scm/bar-line.scm:67: (define-public (bar-line::non-empty-barline grob)
where do you use this?

[marc, 2012-07-21 08:04:27]

Am 20.07.2012 21:15, schrieb Benkő Pál:
> Marc, please don't throw the whole 2533 issue stuff out;
> look at the newer version at
> http://codereview.appspot.com/6431044
>
> p
>
Pál, thanks for the hint – I stored the changes I made
before the revert of your patch ;-)

Obviously, I have do rework some details before my patch gets
accepted, and I think that in the meantime yours will
get through the revision process. Then I'll recode your
work in scheme and implement it in scm/bar-line.scm.

(I didn't think that 1320 will be so hard, and this is
only part 1 of 2 :( )

Regards,

Marc

[marc, 2012-07-21 10:07:01]

Am 20.07.2012 23:16, schrieb k-ohara5a5a@oco.net:
> Works fine for me, but it will need updated to synchronize with the
> patches soon-to-be-pushed to the C version.
Ok.
>
> Since the C code is getting bug-fixes and improvements at the same time
> as you port to Scheme, you should incorporate the improved behavior into
> your Scheme code as soon as you agree that it is correct.  When the
> changed C code is pushed, you will need to rebase your patch, but the
> state of code we are reviewing will not change so often.
>
> Marks and bar-numbers above repeat bars now seem to align to the center
> of the repeat bar including the dots, where before they aligned to just
> the double bar portion. Probably the reference point of the stencil has
> changed.
It was a erroneous calculation in bar-line::calc-anchor which
I corrected in the last upload.
>
> The other regtest changes look acceptable to me, and it seems to work
> essentially as fast as the C code on real music.
>
>
> http://codereview.appspot.com/6305115/diff/30001/lily/bar-line.cc
> File lily/bar-line.cc (left):
>
> http://codereview.appspot.com/6305115/diff/30001/lily/bar-line.cc#oldcode66 
>
> lily/bar-line.cc:66: Bar_line::non_empty_barline (Grob *me)
> Good to keep this in C.  It used to live in paper-column.cc until the
> fix to issue 915.  Later, if we want to remove the file completely, it
> could go back there, later.
Ok.
>
> http://codereview.appspot.com/6305115/diff/30001/lily/bar-line.cc#oldcode426 
>
> lily/bar-line.cc:426: ADD_INTERFACE (Bar_line,
> Would this make more sense in define-grob-interfaces.scm ?
> It would become the largest docstring in that file, but I don't see any
> problem with that.
Done. The docstring will be changed anyway during part II of the
"Big Bar Line Rewriting And Making Life Easier For Everybody" project ;-)
>
> http://codereview.appspot.com/6305115/diff/30001/lily/grob-scheme.cc
> File lily/grob-scheme.cc (right):
>
>
http://codereview.appspot.com/6305115/diff/30001/lily/grob-scheme.cc#newcode459

>
> lily/grob-scheme.cc:459: "Compare two grobs.")
> "Does @var{a} lie above @var{b} on the page?"
Done.
>
> http://codereview.appspot.com/6305115/diff/30001/scm/bar-line.scm
> File scm/bar-line.scm (right):
>
> http://codereview.appspot.com/6305115/diff/30001/scm/bar-line.scm#newcode67 
>
> scm/bar-line.scm:67: (define-public (bar-line::non-empty-barline grob)
> where do you use this?
Removed - thanks for spotting this. This is a leftover from the attempt
to move bar-line.cc *completely* into Scheme.

Regards,

Marc
>
> http://codereview.appspot.com/6305115/
>

[marc, 2012-07-21 10:07:37]

Am 20.07.2012 21:03, schrieb dak@gnu.org:
>
> http://codereview.appspot.com/6305115/diff/30001/scm/bar-line.scm
> File scm/bar-line.scm (right):
>
> http://codereview.appspot.com/6305115/diff/30001/scm/bar-line.scm#newcode649 
>
> scm/bar-line.scm:649:
> Empty line at end of file makes git complain about whitespace error.
Done.

Regards,

Marc
>
> http://codereview.appspot.com/6305115/
>

[Keith, 2012-07-23 21:45:12]

Looks like LilyPond defines two line-thicknesses, 
one for staff-line thickness in each StaffSymbol, 
and one (confusingly called staffline in C) for bar-line width in paper.scm.

If the barlines use the layout thickness, as they mostly did in the C version,
then they line up between staves of different staffline thicknesses.

http://codereview.appspot.com/6305115/diff/46004/scm/bar-line.scm
File scm/bar-line.scm (right):

http://codereview.appspot.com/6305115/diff/46004/scm/bar-line.scm#newcode144
scm/bar-line.scm:144: (stafflinethick (ly:staff-symbol-line-thickness grob))
this one was not like the others in the C version, but maybe that was a bug.

http://codereview.appspot.com/6305115/diff/46004/scm/bar-line.scm#newcode155
scm/bar-line.scm:155: (line-thickness (ly:staff-symbol-line-thickness grob))
unused ?

http://codereview.appspot.com/6305115/diff/46004/scm/bar-line.scm#newcode207
scm/bar-line.scm:207: (line-thickness (ly:staff-symbol-line-thickness grob))
From layout, or just the value 'thickness' passed in ?

http://codereview.appspot.com/6305115/diff/46004/scm/bar-line.scm#newcode257
scm/bar-line.scm:257: (let* ((line-thickness (ly:staff-symbol-line-thickness
grob))
from layout.

http://codereview.appspot.com/6305115/diff/46004/scm/bar-line.scm#newcode339
scm/bar-line.scm:339: (line-thickness (ly:staff-symbol-line-thickness grob)))
Confirming, yes, from staff-symbol because it refers to staffline thickness

http://codereview.appspot.com/6305115/diff/46004/scm/bar-line.scm#newcode372
scm/bar-line.scm:372: (let* ((line-thickness (ly:staff-symbol-line-thickness
grob))
from layout

http://codereview.appspot.com/6305115/diff/46004/scm/bar-line.scm#newcode464
scm/bar-line.scm:464: (let* ((line-thickness (ly:staff-symbol-line-thickness
grob))
the C version used the line-thickness from paper.scm. That is, thickness of
vertical lines comes from layout, not from staff.

[marc, 2012-07-24 09:41:33]

Am 23.07.2012 23:45, schrieb k-ohara5a5a@oco.net:
> Looks like LilyPond defines two line-thicknesses,
> one for staff-line thickness in each StaffSymbol,
> and one (confusingly called staffline in C) for bar-line width in
> paper.scm.
Thanks for your explanation! Indeed, staffline is not the
best variable name in this case, I changed it accordingly (see below).
>
> If the barlines use the layout thickness, as they mostly did in the C
> version, then they line up between staves of different staffline
> thicknesses.
... and the difference in input/regression/staff-tweak.ly is gone!
>
>
> http://codereview.appspot.com/6305115/diff/46004/scm/bar-line.scm
> File scm/bar-line.scm (right):
>
> http://codereview.appspot.com/6305115/diff/46004/scm/bar-line.scm#newcode144 
>
> scm/bar-line.scm:144: (stafflinethick (ly:staff-symbol-line-thickness
> grob))
> this one was not like the others in the C version, but maybe that was a
> bug.
I am not sure either, so I left it unchanged.
>
> http://codereview.appspot.com/6305115/diff/46004/scm/bar-line.scm#newcode155 
>
> scm/bar-line.scm:155: (line-thickness (ly:staff-symbol-line-thickness
> grob))
> unused ?
Removed.
>
> http://codereview.appspot.com/6305115/diff/46004/scm/bar-line.scm#newcode207 
>
> scm/bar-line.scm:207: (line-thickness (ly:staff-symbol-line-thickness
> grob))
> From layout, or just the value 'thickness' passed in ?
Done.
>
> http://codereview.appspot.com/6305115/diff/46004/scm/bar-line.scm#newcode257 
>
> scm/bar-line.scm:257: (let* ((line-thickness
> (ly:staff-symbol-line-thickness grob))
> from layout.
Done.
>
> http://codereview.appspot.com/6305115/diff/46004/scm/bar-line.scm#newcode339 
>
> scm/bar-line.scm:339: (line-thickness (ly:staff-symbol-line-thickness
> grob)))
> Confirming, yes, from staff-symbol because it refers to staffline
> thickness
I renamed it to staff-line-thickness to emphasize the difference.
>
> http://codereview.appspot.com/6305115/diff/46004/scm/bar-line.scm#newcode372 
>
> scm/bar-line.scm:372: (let* ((line-thickness
> (ly:staff-symbol-line-thickness grob))
> from layout
Done.
>
> http://codereview.appspot.com/6305115/diff/46004/scm/bar-line.scm#newcode464 
>
> scm/bar-line.scm:464: (let* ((line-thickness
> (ly:staff-symbol-line-thickness grob))
> the C version used the line-thickness from paper.scm. That is, thickness
> of vertical lines comes from layout, not from staff.
Done.

>
> http://codereview.appspot.com/6305115/
>

[benko.pal, 2012-07-26 07:51:56]

Marc, please don't throw the whole 2533 issue stuff out; look at the latest
version at
http://codereview.appspot.com/6431044
in particular bar-line.cc and repeat-sign.ly.

I really don't mind if your patch goes before mine (it would even be better -
why push a change to a file that is to be thrown out by the next commit), but I
do mind that (some of) those cases work right, and unfortunately I'm far less
fluent in Scheme than in C++.

[marc, 2012-07-26 08:26:43]

Am 26.07.2012 09:51, schrieb benko.pal@gmail.com:
> Marc, please don't throw the whole 2533 issue stuff out; look at the
> latest version at
> http://codereview.appspot.com/6431044
> in particular bar-line.cc and repeat-sign.ly.
>
> I really don't mind if your patch goes before mine (it would even be
> better - why push a change to a file that is to be thrown out by the
> next commit), but I do mind that (some of) those cases work right, and
> unfortunately I'm far less fluent in Scheme than in C++.
>
> http://codereview.appspot.com/6305115/
>
I wanted to contact you anyway – I'd propose that you remove these parts 
of your
patch; when your algorithm is right (I have no doubt about that), I can 
do the c->scheme
translation and do this part of work for you, including the regtest.

For me, it was important that my patch reflects the status-quo of the 
bar line
stuff; changes will be far easier to do in the future.

What do you think?

Regards,

Marc

[Keith, 2012-07-26 08:29:14]

On Thu, 26 Jul 2012 00:51:56 -0700, <benko.pal@gmail.com> wrote:

> Marc, please don't throw the whole 2533 issue stuff out; look at the
> latest version at
> http://codereview.appspot.com/6431044
> in particular bar-line.cc and repeat-sign.ly.
>
> I really don't mind if your patch goes before mine (it would even be
> better - why push a change to a file that is to be thrown out by the
> next commit), but I do mind that (some of) those cases work right, and
> unfortunately I'm far less fluent in Scheme than in C++.
>

I had suggested that Marc incorporate pending changes to the bar-line code
if he is confident in them,
but it does seem cleaner make one commit a simple re-implementation of C to
Scheme, with the same functionality.

He is not really throwing out the issue-2533 stuff, because that stuff is
not in the code base at the moment.

You will just have to cooperate with Marc or another Scheme programmer, if
the re-implementation goes in first.  If it really turns out to be harder
for the group to modify the code in Scheme than it was in C, then the reason
for Marc's patch would prove to be wrong and we can just go back to C.

[marc, 2012-07-26 08:36:58]

Am 26.07.2012 10:31, schrieb Keith OHara:
> On Thu, 26 Jul 2012 00:51:56 -0700, <benko.pal@gmail.com> wrote:
>
>> Marc, please don't throw the whole 2533 issue stuff out; look at the
>> latest version at
>> http://codereview.appspot.com/6431044
>> in particular bar-line.cc and repeat-sign.ly.
>>
>> I really don't mind if your patch goes before mine (it would even be
>> better - why push a change to a file that is to be thrown out by the
>> next commit), but I do mind that (some of) those cases work right, and
>> unfortunately I'm far less fluent in Scheme than in C++.
>>
>
> I had suggested that Marc incorporate pending changes to the bar-line 
> code
> if he is confident in them,
> but it does seem cleaner make one commit a simple re-implementation of 
> C to
> Scheme, with the same functionality.
>
> He is not really throwing out the issue-2533 stuff, because that stuff is
> not in the code base at the moment.
>
> You will just have to cooperate with Marc or another Scheme 
> programmer, if
> the re-implementation goes in first.  If it really turns out to be harder
> for the group to modify the code in Scheme than it was in C, then the 
> reason
> for Marc's patch would prove to be wrong and we can just go back to C.
I had already implemented Páls work on repeat signs *before* his patch
was reverted, so reimplementing this in scheme should be straightforward.

... and wait until you see what will be possible with barlines when part 
2 of
issue 1320 is out ;-) Stay tuned!

Regards,

Marc

>
>

[benko.pal, 2012-07-26 15:35:57]

2012/7/26 Marc Hohl <marc@hohlart.de>:
> Am 26.07.2012 10:31, schrieb Keith OHara:
>
>> On Thu, 26 Jul 2012 00:51:56 -0700, <benko.pal@gmail.com> wrote:
>>
>>> Marc, please don't throw the whole 2533 issue stuff out; look at the
>>> latest version at
>>> http://codereview.appspot.com/6431044
>>> in particular bar-line.cc and repeat-sign.ly.
>>>
>>> I really don't mind if your patch goes before mine (it would even be
>>> better - why push a change to a file that is to be thrown out by the
>>> next commit), but I do mind that (some of) those cases work right, and
>>> unfortunately I'm far less fluent in Scheme than in C++.
>>>
>>
>> I had suggested that Marc incorporate pending changes to the bar-line code
>> if he is confident in them,
>> but it does seem cleaner make one commit a simple re-implementation of C
>> to Scheme, with the same functionality.
>>
>> He is not really throwing out the issue-2533 stuff, because that stuff is
>> not in the code base at the moment.
>>
>> You will just have to cooperate with Marc or another Scheme programmer, if
>> the re-implementation goes in first.  If it really turns out to be harder
>> for the group to modify the code in Scheme than it was in C, then the
>> reason
>> for Marc's patch would prove to be wrong and we can just go back to C.
>
> I had already implemented Páls work on repeat signs *before* his patch
> was reverted, so reimplementing this in scheme should be straightforward.
>
> ... and wait until you see what will be possible with barlines when part 2
> of issue 1320 is out ;-) Stay tuned!

ok, thanks in advance to everyone!