Fixes problems with glissando line breaking code pointed out by Han-Wen.

Fixes problems with glissando line breaking code pointed out by Han-Wen.

[MikeSol, 2011-07-01 15:23:28]

Cheers,
MS

[Neil Puttock, 2011-07-01 17:21:35]

Hi Mike,

This breaks unbroken glissandos since you can't rely on ly:grob-original to
distinguish between split/unsplit spanners.  I suggest using
ly:spanner-broken-into instead: if it's a pair, you're good to go, otherwise
don't do anything.

Cheers,
Neil

[mike_apollinemike.com, 2011-07-01 17:27:14]

Hey Neil,

Sorry - I don't completely understand what you mean.  This patch currently uses
ly:spanner-broken-into - do you mean that it should be used in a different way?

Cheers,
MS

On Jul 1, 2011, at 7:21 PM, n.puttock@gmail.com wrote:

> Hi Mike,
> 
> This breaks unbroken glissandos since you can't rely on ly:grob-original
> to distinguish between split/unsplit spanners.  I suggest using
> ly:spanner-broken-into instead: if it's a pair, you're good to go,
> otherwise don't do anything.
> 
> Cheers,
> Neil
> 
> 
> http://codereview.appspot.com/4634119/

[Neil Puttock, 2011-07-01 17:40:37]

On 2011/07/01 17:27:14, mike_apollinemike.com wrote:

> Sorry - I don't completely understand what you mean.  This patch currently
uses
> ly:spanner-broken-into - do you mean that it should be used in a different
way?

(define-public (glissando::make-simple-y grob)
  "Establishes the Y terminus points of the glissando based on the pre-broken
positions of its left and right bounds."
  (let* ((siblings (ly:spanner-broken-into (ly:grob-original grob)))
         (bound-details (ly:grob-property grob 'bound-details))
         (extra-dy (ly:grob-property grob 'extra-dy 0.0)))

    (and (pair? siblings)
         (for-each (lambda (dir-sym)
                     (let* ((details (assoc-get dir-sym bound-details))
                            (dir (if (eq? dir-sym 'left) LEFT RIGHT))
                            (good-grob (if (eq? dir-sym 'left)
                                           (first siblings)
                                           (last siblings)))
                            (bound (ly:spanner-bound good-grob dir))
                            (common-y (ly:grob-common-refpoint good-grob bound
Y))
                            (y (+ (interval-center (ly:grob-extent bound
common-y Y))
                                  (/ (* dir extra-dy)
                                     2))))
                       (if (not (assoc-get 'Y details))
                           (set! bound-details (assoc-set! bound-details dir-sym
                                                           (acons 'Y y
details))))))
                   '(left right))

         (set! (ly:grob-property grob 'bound-details) bound-details))))

[mike_apollinemike.com, 2011-07-02 21:16:21]

On Jul 1, 2011, at 7:21 PM, n.puttock@gmail.com wrote:

> Hi Mike,
> 
> This breaks unbroken glissandos since you can't rely on ly:grob-original
> to distinguish between split/unsplit spanners.  I suggest using
> ly:spanner-broken-into instead: if it's a pair, you're good to go,
> otherwise don't do anything.
> 
> Cheers,
> Neil
> 

Thanks for your help Neil, and thanks to Han-Wen for spotting the issue with
line breaks.
The original code broke LilyPond in a very bad way - all documents printed
glissandi as if they had the Y-extent of the first glissando in a piece.  I have
no clue why this happened, but the new code doesn't do this, and I've modified
the regtest to check for this.

After getting clean regtests, the change is pushed as
1dd946408282051972bc75c849cd406670174da1.

Cheers,
MS

[Neil Puttock, 2011-07-03 13:36:36]

On 2 July 2011 22:16, mike@apollinemike.com <mike@apollinemike.com> wrote:

> The original code broke LilyPond in a very bad way - all documents printed
glissandi as if they had the Y-extent of the first glissando in a piece.  I have
no clue why this happened, but the new code doesn't do this, and I've modified
the regtest to check for this.

It's still broken here.

Beware of the regression testing for this: glissandos are completely
ignored since they have empty extents.

Cheers,
Neil

[mike_apollinemike.com, 2011-07-03 17:09:30]

On Jul 3, 2011, at 3:36 PM, Neil Puttock wrote:

> On 2 July 2011 22:16, mike@apollinemike.com <mike@apollinemike.com> wrote:
> 
>> The original code broke LilyPond in a very bad way - all documents printed
glissandi as if they had the Y-extent of the first glissando in a piece.  I have
no clue why this happened, but the new code doesn't do this, and I've modified
the regtest to check for this.
> 
> It's still broken here.
> 
> Beware of the regression testing for this: glissandos are completely
> ignored since they have empty extents.
> 
> Cheers,
> Neil

Thanks for the info!  I had no idea this was the case - otherwise I would have
looked at it manually.  Perhaps this is worth noting in the contributor's guide?
The issue in the current code is that every glissando receives the left and
right Y bounds of the last glissando typeset.  I'll be able to delve into this
tomorrow - my first stab at it will be rewriting the algorithm in C++ to see if
it makes a difference.

Cheers,
MS

[Neil Puttock, 2011-07-03 18:04:11]

On 3 July 2011 18:09, mike@apollinemike.com <mike@apollinemike.com> wrote:

> Thanks for the info!  I had no idea this was the case - otherwise I would have
looked at it manually.  Perhaps this is worth noting in the contributor's guide?

The CG does mention bounding boxes, so it is noted in a roundabout way.

> The issue in the current code is that every glissando receives the left and
right Y bounds of the last glissando typeset.  I'll be able to delve into this
tomorrow - my first stab at it will be rewriting the algorithm in C++ to see if
it makes a difference.

It looks like the alist is shared between glissandos (perhaps an
artefact of using assoc-set! rather than building a completely new
alist like you originally did).

It works fine if you use an after-line-breaking callback and set Y
directly via ly:grob-set-nested-property! (though judging by Han-Wen's
comments, this is an invalid solution.)

Cheers,
Neil

[Neil Puttock, 2011-07-03 19:22:09]

On 3 July 2011 19:04, Neil Puttock <n.puttock@gmail.com> wrote:

> It works fine if you use an after-line-breaking callback and set Y
> directly via ly:grob-set-nested-property! (though judging by Han-Wen's
> comments, this is an invalid solution.)

Hah, it also works fine if you use left/right-bound-info (as I
originally suggested. ;)

Cheers,
Neil