[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: |
mike |
Subject: |
Re: Uses single algorithm for side-position spacing. (issue 6827072) |
Date: |
Wed, 14 Nov 2012 07:33:11 +0100 |
> Re: Uses single algorithm for side-position spacing. (issue 6827072)
>
> {\clef bass
> <g^3 a^5>2..->
> << r16 \\ r \\ r \\ r \\ >> eeses'16
> \set fingeringOrientations = #'(right)
> <c e>8-1-4 <c^1 e^4> <g,-3 b,-4> r
> r2 }
>
Beautiful ugly test case. Even with current master it is atrocious. I've fixed
everything but the double-flat on rest collision, which'd require a separate
patch.
>
> http://codereview.appspot.com/6827072/diff/1/input/regression/dynamics-avoid-cross-staff-stem.ly
>
> File input/regression/dynamics-avoid-cross-staff-stem.ly (right):
>
>
> http://codereview.appspot.com/6827072/diff/1/input/regression/dynamics-avoid-cross-staff-stem.ly#newcode14
>
> input/regression/dynamics-avoid-cross-staff-stem.ly:14: a8 \p [ \change
> Staff = "PnRH" \stemDown gis'8 \fff ]
> Before your patch, the dynamics below the staff clear each other; after
> your patch, they collide.
>
Fixed.
>
> http://codereview.appspot.com/6827072/diff/1/lily/axis-group-interface.cc
>
> File lily/axis-group-interface.cc (right):
>
>
> http://codereview.appspot.com/6827072/diff/1/lily/axis-group-interface.cc#newcode403
>
> lily/axis-group-interface.cc:403: Axis_group_interface::cross_staff (SCM
> smob)
> For what situation? Which object that supports axis-group-interface
> (PianoPedalSpanner, DynamicLineSpanner) should be potentially considered
> a cross-staff object?
NoteColumn
> http://codereview.appspot.com/6827072/diff/1/lily/box-quarantine.cc
>
> File lily/box-quarantine.cc (right):
>
>
> http://codereview.appspot.com/6827072/diff/1/lily/box-quarantine.cc#newcode70
>
> lily/box-quarantine.cc:70: return abs (ii0.index_ - mid) < abs
> (ii1.index_ - mid);
> gcc 4.5.1 complains that he cannot determine which version of the
> overloaded abs() should be called. (Polymorphism is l33t, isn't it.) I
> just removed the abs() calls to get it to compile, because index_ is an
> unsigned type so the arguments are always positive anyway.
index_ is always positive and mid_ is always positive but index_ - mid_ will be
positive or negative depending on which one is larger. Changing to fabs.
>
>
> http://codereview.appspot.com/6827072/diff/1/lily/script-engraver.cc
>
> File lily/script-engraver.cc (right):
>
>
> http://codereview.appspot.com/6827072/diff/1/lily/script-engraver.cc#newcode270
>
> lily/script-engraver.cc:270: // we never want scripts to be tucked down
> next to stems
> This disagrees with
> input/regression/finger-chords.ly
True - I mean Script grobs. I'll make that clearer.
>
>
> http://codereview.appspot.com/6827072/diff/1/lily/stencil-integral.cc
>
> File lily/stencil-integral.cc (right):
>
>
> http://codereview.appspot.com/6827072/diff/1/lily/stencil-integral.cc#newcode988
>
> lily/stencil-integral.cc:988: pure = Rest::has_interface (me) ? true :
> pure;
> Worse than losing ledgers, the 'pure' extent is in the wrong position
> for those rests that might have been moved.
>
> At this point in determining the layout, shouldn't you ensure that the
> rests are positioned, by testing 'positioning-done, before figuring
> their skylines for use in note-spacing ?
>
Even this doesn't work. It fails maybe every 5th time I compile, so it must be
one of those uniq () sorting problems that I run into from time to time. The
solution for now is just to use ly:grob::vertical-skylines-from-stencil on the
rests, which don't muddle with extents or positioning and just check the glyph.
But this is me going into problem-avoidance mode. There are comments all over
that part of the code base warning about RestColumn positioning, but a big fat
"THIS IS A TICKING CIRCULAR DEPENDENCY TIME BOMB" comment somewhere in there
wouldn't hurt.
>
> http://codereview.appspot.com/6827072/diff/1/scm/define-grob-properties.scm
>
> File scm/define-grob-properties.scm (right):
>
>
> http://codereview.appspot.com/6827072/diff/1/scm/define-grob-properties.scm#newcode668
>
> scm/define-grob-properties.scm:668: (other-axis-padding ,number? "The
> amount to pad the axis
> traditionally called horizon-padding, which I suppose makes sense in the
> metaphor of a city skyline, as this padding is in the direction of the
> horizon.
>
>
Fixed.
Thanks for the review! Will upload a new patch set in the not-too-distant
future.
Cheers,
MS
Re: Uses single algorithm for side-position spacing. (issue 6827072), k-ohara5a5a, 2012/11/12
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), 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