[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: Uses single algorithm for side-position spacing. (issue 6827072)
From: |
address@hidden |
Subject: |
Re: Uses single algorithm for side-position spacing. (issue 6827072) |
Date: |
Thu, 29 Nov 2012 09:27:42 +0100 |
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/
- Re: Uses single algorithm for side-position spacing. (issue 6827072), (continued)
- Re: Uses single algorithm for side-position spacing. (issue 6827072), address@hidden, 2012/11/18
- Re: Uses single algorithm for side-position spacing. (issue 6827072), Keith OHara, 2012/11/18
- Re: Uses single algorithm for side-position spacing. (issue 6827072), address@hidden, 2012/11/18
- Re: Uses single algorithm for side-position spacing. (issue 6827072), Keith OHara, 2012/11/18
- Re: Uses single algorithm for side-position spacing. (issue 6827072), address@hidden, 2012/11/23
- Re: Uses single algorithm for side-position spacing. (issue 6827072), mike, 2012/11/23
Re: Uses single algorithm for side-position spacing. (issue 6827072), k-ohara5a5a, 2012/11/17
Re: Uses single algorithm for side-position spacing. (issue 6827072), k-ohara5a5a, 2012/11/17
Re: Uses single algorithm for side-position spacing. (issue 6827072), k-ohara5a5a, 2012/11/28
- Re: Uses single algorithm for side-position spacing. (issue 6827072),
address@hidden <=
Re: Uses single algorithm for side-position spacing. (issue 6827072), k-ohara5a5a, 2012/11/29
Re: Uses single algorithm for side-position spacing. (issue 6827072), k-ohara5a5a, 2012/11/29
Re: Uses single algorithm for side-position spacing. (issue 6827072), k-ohara5a5a, 2012/11/30
Re: Uses single algorithm for side-position spacing. (issue 6827072), k-ohara5a5a, 2012/11/30