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

[address@hidden]

On 29 nov. 2012, at 03:23, address@hidden 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/