Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 29 nov. 2012, at 03:23, k-ohara5...@oco.net wrote:

> 
> http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc
> File lily/box-quarantine.cc (right):
> 
> http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc#newcode69
> lily/box-quarantine.cc:69: int mid = ii0.mid_;
>  assert ((double)(ii0.index - mid) >= 0.0)
>  assert ((double)(ii1.index - mid) >= 0.0)
> 

This will fail often...it is possible, for example, that ii0.index is 0 and mid 
is 3 (mid represents the middle index of the vector).

> http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc#newcode93
> lily/box-quarantine.cc:93: Offset shift (-(epsilon + (ii[0].amount_ +
> padding_) / 2.0), 0.0);
> The padding appears in some places but not others; see
> 'fingering-column.ly'
> 

Fixed.

> http://codereview.appspot.com/6827072/diff/18003/lily/skyline.cc
> File lily/skyline.cc (right):
> 
> http://codereview.appspot.com/6827072/diff/18003/lily/skyline.cc#newcode234
> lily/skyline.cc:234: // Flatten to height h
> What is the difference between this and Skyline:::set_minimum_height()
> at line 817 ?
> 

Set minimum height allows for things over the minimum height to retain their 
skyline-ness, whereas this creates a flat skyline at height X.

> http://codereview.appspot.com/6827072/diff/18003/scm/define-grob-properties.scm
> File scm/define-grob-properties.scm (right):
> 
> http://codereview.appspot.com/6827072/diff/18003/scm/define-grob-properties.scm#newcode492
> scm/define-grob-properties.scm:492: (horizon-padding ,number? "The
> amount to pad the axis
> We already have 'skyline-horizontal-padding' on line 815 and
> 'skyline-vertical-padding', so it would be better to use those.  You
> could look up one or the other depending on which axis, X or Y, in which
> the buildings grow.
> 
> http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm
> File scm/define-grobs.scm (right):
> 
> http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm#newcode253
> scm/define-grobs.scm:253: (horizon-padding . 0.05)
> This could be 'skyline-horizontal-padding' as in System and
> VerticalAxisGroup.
> 

I definitely agree, but this'd be for another patch set.  The eventual goal of 
all of this is to get all spacing out of axis-group-interface and into side 
position interface via one general algorithm.  When that happens, I'll make 
these merges.  For now, it is a bit difficult, as there are two concurrent 
systems - one that specifies vertical versus horizontal padding and one that 
specifies padding versus horizon-padding.  The latter system (side-position) 
probably needs to be changed, but it'd require a major convert-ly rule.

> http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm#newcode886
> scm/define-grobs.scm:886: (add-stem-support . #t)
> Several regression tests assume this is false by default, and then
> toggle it to true, so as to test both cases.
> 
> add-stem-support = #f doesn't seem to work very well with beams with
> this patch.
> 

One can write a lambda function that returns #t if there is a beam present and 
#f otherwise.  Otherwise, the beam would have to be added at the engraver 
stage.  I prefer the former.

> http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm#newcode909
> scm/define-grobs.scm:909: (FingeringCollision
> This would need a convert-ly rule.  Why change the name ?
> 

You recommended that in a previous review. I can push the convert-ly rule as a 
separate patch.

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


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

On 2012/11/29 08:28:31, mike7 wrote:

On 29 nov. 2012, at 03:23, mailto:k-ohara5...@oco.net wrote:



http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc#newcode69

> lily/box-quarantine.cc:69: int mid = ii0.mid_;
>  assert ((double)(ii0.index - mid) >= 0.0)



it is possible, for example, that ii0.index is 0 and mid
is 3 (mid represents the middle index of the vector).


  ii0.index_ = 0;
  mid = 3;
  printf("%f\n",(double)(ii0.index_ - mid));

4294967293.00



> add-stem-support = #f doesn't seem to work very well with beams
> with this patch.



One can write a lambda function ...


But I want to set Wilder Reiter (my favorite piece when I was eight, and
the example we discussed where fingerings go alongside stems)

and I don't know how to write lambda functions.


http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm#newcode909

> scm/define-grobs.scm:909: (FingeringCollision
> This would need a convert-ly rule.  Why change the name ?
>



You recommended that in a previous review.


Well I must have been suffering from a case of the stupids, because
FingeringColumn was a perfect name for a column of fingering
indications.   Now I'm suffering a case of amnesia trying to think of
what I could have meant.

http://codereview.appspot.com/6827072/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

It was Box_quarantine that seemed a confusing name. (Why 'quarantine'?
Is there something special about 40 boxes?)  I take back my suggestion.


http://codereview.appspot.com/6827072/diff/7041/lily/box-quarantine.cc
File lily/box-quarantine.cc (right):

http://codereview.appspot.com/6827072/diff/7041/lily/box-quarantine.cc#newcode63
lily/box-quarantine.cc:63: TODO: not sure if this loop causes infinite
behavior...
The while(dirty) loop runs 2366 times for the last chord in
'fingering-collision.ly' but that's an extreme case.

http://codereview.appspot.com/6827072/diff/7041/lily/fingering-column.cc
File lily/fingering-column.cc (left):

http://codereview.appspot.com/6827072/diff/7041/lily/fingering-column.cc#oldcode63
lily/fingering-column.cc:63: fingerings[i]->translate_axis
(-fingerings[i]->extent (common[Y_AXIS], Y_AXIS).length () / 2, Y_AXIS);
This shifted all the fingerings down to align them horizontally to their
note-heads, and is missing from the new code

http://codereview.appspot.com/6827072/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 29 nov. 2012, at 10:24, k-ohara5...@oco.net wrote:

> It was Box_quarantine that seemed a confusing name. (Why 'quarantine'?
> Is there something special about 40 boxes?)  I take back my suggestion.
> 
> 
> http://codereview.appspot.com/6827072/diff/7041/lily/box-quarantine.cc
> File lily/box-quarantine.cc (right):
> 
> http://codereview.appspot.com/6827072/diff/7041/lily/box-quarantine.cc#newcode63
> lily/box-quarantine.cc:63: TODO: not sure if this loop causes infinite
> behavior...
> The while(dirty) loop runs 2366 times for the last chord in
> 'fingering-collision.ly' but that's an extreme case.

I'm not proud of this...

> 
> http://codereview.appspot.com/6827072/diff/7041/lily/fingering-column.cc
> File lily/fingering-column.cc (left):
> 
> http://codereview.appspot.com/6827072/diff/7041/lily/fingering-column.cc#oldcode63
> lily/fingering-column.cc:63: fingerings[i]->translate_axis
> (-fingerings[i]->extent (common[Y_AXIS], Y_AXIS).length () / 2, Y_AXIS);
> This shifted all the fingerings down to align them horizontally to their
> note-heads, and is missing from the new code
> 

Fixed.

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


___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[m...@mikesolomon.org]

On 29 nov. 2012, at 10:13, k-ohara5...@oco.net wrote:

> On 2012/11/29 08:28:31, mike7 wrote:
>> On 29 nov. 2012, at 03:23, mailto:k-ohara5...@oco.net wrote:
> 
> 
> http://codereview.appspot.com/6827072/diff/18003/lily/box-quarantine.cc#newcode69
>> > lily/box-quarantine.cc:69: int mid = ii0.mid_;
>> >  assert ((double)(ii0.index - mid) >= 0.0)
> 
>> it is possible, for example, that ii0.index is 0 and mid
>> is 3 (mid represents the middle index of the vector).
> 
>  ii0.index_ = 0;
>  mid = 3;
>  printf("%f\n",(double)(ii0.index_ - mid));
> 
> 4294967293.00
> 

woah...totally over my head, but ok, convinced

> 
>> > add-stem-support = #f doesn't seem to work very well with beams
>> > with this patch.
> 
>> One can write a lambda function ...
> 
> But I want to set Wilder Reiter (my favorite piece when I was eight, and
> the example we discussed where fingerings go alongside stems)
> 
> and I don't know how to write lambda functions.
> 

I wrote a lambda function. Regest added.

> 
> http://codereview.appspot.com/6827072/diff/18003/scm/define-grobs.scm#newcode909
>> > scm/define-grobs.scm:909: (FingeringCollision
>> > This would need a convert-ly rule.  Why change the name ?
>> >
> 
>> You recommended that in a previous review.
> 
> Well I must have been suffering from a case of the stupids, because
> FingeringColumn was a perfect name for a column of fingering
> indications.   Now I'm suffering a case of amnesia trying to think of
> what I could have meant.


from days of yore...

http://codereview.appspot.com/6827072/diff/11002/lily/box-quarantine.cc#newcode24
lily/box-quarantine.cc:24: Box_quarantine::Box_quarantine (Real padding,
Axis a)
So far it looks like this handles fingering only.  Other similar
problems are handled with a XX_Collision object, so maybe just call this
Fingering_Collision ?___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: markup-commands rest-by-number and rest (issue 6850073)

[benko . pal]

On 2012/11/29 00:41:14, thomasmorley65 wrote:

On 2012/11/27 20:03:06, benko.pal wrote:



> > Did you had a look on the compiled output of the new reg-tests?


I did now, it's OK with me.


https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest-styles.ly
File input/regression/markup-rest-styles.ly (right):

https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest-styles.ly#newcode25
input/regression/markup-rest-styles.ly:25: '(-3 -2 -1 0 1 2 3 4 5 6
7
please fix indentation, this and the next list are at quite different
levels.  similarly in the other .ly file.

https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest.ly
File input/regression/markup-rest.ly (right):

https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest.ly#newcode27
input/regression/markup-rest.ly:27: (if (>= duration 0) dots "")
this if looks superfluous

https://codereview.appspot.com/6850073/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

still absent

[Janek Warchoł]

Dear friends,

i hope that you're doing well.
i have to remain off-list at least until Christmas :(

all the best,
Janek

PS the conductor of my choir is in critical state after heart surgery
complications.  please, pray for him if you can.
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

How to do proper indentation?

[Thomas Morley]

Hi,

reviewing
https://codereview.appspot.com/6850073/
Pál admonished to do better indentation on lists at different levels.

Trying to minimize my lack of knowledge on this topic I searched the
CG about It.

CG 10.5.3 Indentation
mentions possibilities for emacs and vim, but I'm not familiar with them.
I'm using jEdit or gedit

Following the link in
CG 10.3.2 Desired file formatting
http://community.schemewiki.org/?scheme-style
doesn't help here.

My own guess would be (file is attached, too):

showSimpleRest =
#(define-scheme-function (parser location dots)
  (string?)
  (make-override-markup (cons 'baseline-skip 7)
   (make-column-markup
(map
 (lambda (style)
  (make-line-markup
   (list
(make-pad-to-box-markup '(0 . 20) '(0 . 0)
 (symbol->string style))
  (make-override-markup (cons 'line-width 60)
   (make-override-markup (cons 'style style)
(make-fill-line-markup
 (map
  (lambda (duration)
   (make-rest-markup
(if (string? duration)
  duration
  (string-append
   (number->string (expt 2 duration))
   (if (>= duration 0) dots "")
 (append '("maxima" "longa" "breve")(iota 8)
'(default
  mensural
  neomensural
  classical
  baroque
  altdefault
  petrucci
  blackpetrucci
  semipetrucci
  kievan)

Is this ok?
If not, how to do? Or where to look to learn it?


-Harm


indentation.ly
Description: Binary data
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: markup-commands rest-by-number and rest (issue 6850073)

[thomasmorley65]

On 2012/11/29 20:42:21, benko.pal wrote:

On 2012/11/29 00:41:14, thomasmorley65 wrote:
> On 2012/11/27 20:03:06, benko.pal wrote:
>
> > > Did you had a look on the compiled output of the new reg-tests?



I did now,


Thanks for doing this.


it's OK with me.



https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest-styles.ly

File input/regression/markup-rest-styles.ly (right):



https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest-styles.ly#newcode25

input/regression/markup-rest-styles.ly:25: '(-3 -2 -1 0 1 2 3 4 5 6

7

please fix indentation, this and the next list are at quite different

levels.

similarly in the other .ly file.


Couldn't find a guideline to do correct indentation here.
I ask on -devel


https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest.ly

File input/regression/markup-rest.ly (right):



https://codereview.appspot.com/6850073/diff/7/input/regression/markup-rest.ly#newcode27

input/regression/markup-rest.ly:27: (if (>= duration 0) dots "")
this if looks superfluous


Yep.
Will delete it.



https://codereview.appspot.com/6850073/

___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

PATCH: Countdown to 20121203

[Colin Campbell]

For 22:00 MST Monday December 3rd

Crash:

 Issue 2972 
: Adding 
StringNumber 0 in TabStaff crashes - R 6858077 



Defect:
Issue 732 : 
Alignment problems when vertically stacking horizontally centered 
stencils - R 6855077 


Documentation:
Issue 2916 
: Doc : NR 
Document \hide command - R 6851102 


Enhancement:
Issue 2984 
: Patch: Use 
define-void-function rather than define-music-function in several places 
- R 6854104 
Issue 2982 
: Patch: Remove 
"selective" contextmods. - R 6846107 



Ugly:
Issue 2527 
: Finger-flag 
collision - R 6827072 


Cheers,
Colin
--
Dwy ddim yn cwyno; mae neb yn gwrando!
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel

Re: Uses single algorithm for side-position spacing. (issue 6827072)

[k-ohara5a5a]

'finger-chords.ly' is still in disagreement with its texidoc (therefore
failing).  You could adjust that regtest in light of the new defaults,
of course.

Better might be to make Fingering.add-stem-support = #only-if-beamed
the new default.  That is in better agreement with common practice.
Regtests come out just as good.  (Les-neréides.ly can remove a couple
"tweaks", which seems to be how somebody was keeping score.)  Then users
will less often want to override add-stem-support, and when they do it
will be to the simpler ##f or ##t


> The while(dirty) loop runs 2366 times for the last chord in
> 'fingering-collision.ly' but that's an extreme case.
>
I'm not proud of this...


It is at least comprehensible; while the code it replaced was utterly
baffling.  I simplified the loop


You should at least put the filenames back to what they were, and adjust
any tests or documentation or snippets using add-stem support, before
pushing.

I started to test with real music.  The usual Chopin test case

has a badly broken cross-staff beam in measure 27.  I don't yet see the
cause.

https://codereview.appspot.com/6827072/
___
lilypond-devel mailing list
lilypond-devel@gnu.org
https://lists.gnu.org/mailman/listinfo/lilypond-devel