make sure that AmbitusLine is visible for small ambits

Until now, the gap between ambitus heads and ambitus line
was constant, so either the gap had to be very small or
the line wasn't visible in case of smaller ambits (like
4th or 5th). With this patch ambitus gap will be calculated
dynamically, depending on the distance between ambitus heads.

Also, the default gap for bigger ambits is changed so that
ambitus line will end precisely in the staffline or in the
middle of staffspace.

[Janek Warchol, 2011-06-12 23:10:13]

Oops, i added wrong Mike...

Here is the code i used for testing; i attach a pdf compiled with my fix:

\new Staff \with { \consists Ambitus_engraver } {
    \override Staff.AmbitusLine #'gap = #0.45
    c' f'
}
\new Staff \with { \consists Ambitus_engraver } {
    \override Staff.AmbitusLine #'gap = #0.45
    c' g'
}
\new Staff \with { \consists Ambitus_engraver } {
    \override Staff.AmbitusLine #'gap = #0.45
    c' a'
}
\new Staff \with { \consists Ambitus_engraver } {
    \override Staff.AmbitusLine #'gap = #0.45
    c' b'
}
\new Staff \with { \consists Ambitus_engraver } {
    \override Staff.AmbitusLine #'gap = #0.45
    c' c''
}
\new Staff \with { \consists Ambitus_engraver } {
    \override Staff.AmbitusLine #'gap = #0.45
    a a''
}

cheers,
Janek

[Trevor Daniels, 2011-06-13 08:01:22]

The interpersed comments make it very difficult to read
the code.  Could you place an abridged summary at the top
instead?

[Janek Warchol, 2011-06-13 09:37:31]

On 2011/06/13 08:01:22, Trevor Daniels wrote:
> The interpersed comments make it very difficult to read
> the code.  Could you place an abridged summary at the top
> instead?

Done.
I write a lot of comments so that rookies like me would be able to understand
the code in finite time, if they happen to work on this in the future ;)

[Trevor Daniels, 2011-06-13 11:59:10]

Thanks - much clearer!

Two points:

a) It would be better to honour the value of 'gap if this
is set by the user, rather than change a specifically 
requested gap value.

b) I don't understand why quanting is desired.  An ambitus
doesn't align with anything.  What is your reason?

[Janek Warchol, 2011-06-13 13:52:09]

2011/6/13  <tdanielsmusic@googlemail.com>:
> Thanks - much clearer!
>
> Two points:
>
> a) It would be better to honour the value of 'gap if this
> is set by the user, rather than change a specifically
> requested gap value.

My rationale is that it wouldn't make sense to set a big gap and
really want to have it applied to all ambituses. Consider the
following:

\new Staff \with { \consists Ambitus_engraver } {
    \override Staff.AmbitusLine #'gap = #0.7
    c' g'
}
\new Staff \with { \consists Ambitus_engraver } {
    \override Staff.AmbitusLine #'gap = #0.7
    c' a'
}
\new Staff \with { \consists Ambitus_engraver } {
    \override Staff.AmbitusLine #'gap = #0.7
    c' b'
}
\new Staff \with { \consists Ambitus_engraver } {
    \override Staff.AmbitusLine #'gap = #0.7
    c' c''
}
\new Staff \with { \consists Ambitus_engraver } {
    \override Staff.AmbitusLine #'gap = #0.7
    c' e''
}
\new Staff \with { \consists Ambitus_engraver } {
    \override Staff.AmbitusLine #'gap = #0.7
    a a''
}

While gap=0.7 works fine for big ambituses (one can easily imagine
that a user may wish such a value), the ambitus of sixth looks
ridiculous without any line inside. I suppose that if someone would
like that "look", he would probably switch the line off entirely.
And for small gap the effect is almost unnoticeable.

> b) I don't understand why quanting is desired.  An ambitus
> doesn't align with anything.  What is your reason?

I thought that it's best if ambitus line eihter ends precisely inside
staff line, or protrudes from it distinctly. In other words, i judged
that
\new Staff \with { \consists "Ambitus_engraver" }
    { \override Staff.AmbitusLine #'gap = #0.3 c' b' }
doesn't look nice.
Ofc i'm aware that this is perhaps as nitpicky as it can go :D

Thanks,
Janek

[t.daniels_treda.co.uk, 2011-06-13 15:28:46]

Janek Warchoł wrote Monday, June 13, 2011 2:51 PM

> 2011/6/13  <tdanielsmusic@googlemail.com>:

>> a) It would be better to honour the value of 'gap if this
>> is set by the user, rather than change a specifically
>> requested gap value.
>
> My rationale is that it wouldn't make sense to set a big gap and
> really want to have it applied to all ambituses. Consider the
> following:
>
> \new Staff \with { \consists Ambitus_engraver } {
>    \override Staff.AmbitusLine #'gap = #0.7
>    c' g'
> }
...
> \new Staff \with { \consists Ambitus_engraver } {
>    \override Staff.AmbitusLine #'gap = #0.7
>    a a''
> }
>
> While gap=0.7 works fine for big ambituses (one can easily imagine
> that a user may wish such a value), the ambitus of sixth looks
> ridiculous without any line inside. I suppose that if someone 
> would
> like that "look", he would probably switch the line off entirely.
> And for small gap the effect is almost unnoticeable.

Your algorithm is fine as the default behaviour, but it does remove 
the
ability for a user to precisely set the gap he wants by setting 
'gap,
for whatever reason.  This seems counter to Lily's flexible user 
control,
but I don't feel too strongly about it.  If this is implemented the 
description
of 'gap will have to be changed to explain this.

>> b) I don't understand why quanting is desired. An ambitus
>> doesn't align with anything. What is your reason?
>
> I thought that it's best if ambitus line eihter ends precisely 
> inside
> staff line, or protrudes from it distinctly. In other words, i 
> judged
> that
> \new Staff \with { \consists "Ambitus_engraver" }
>    { \override Staff.AmbitusLine #'gap = #0.3 c' b' }
> doesn't look nice.
> Ofc i'm aware that this is perhaps as nitpicky as it can go :D

OK.  It is! :)   Does it scale correctly with large and small values 
of
global staff-size?

Trevor

[Mike, 2011-06-13 16:30:43]

I think you have the wrong Mike :-)

On Sun, Jun 12, 2011 at 7:04 PM, <lemniskata.bernoullego@gmail.com> wrote:

> Reviewers: Mike,
>
> Description:
> ambitus: special handling of small ambits' lines
>
> Until now, it was not possible to have all ambits
> look good: either the gaps between ambit line
> and heads were too big for ambits of 4th and 5th,
> or they were too small for other ambits.
> This patch introduces automatic scaling
> of the gap between heads and line: bigger ambits
> are left unchanged, but smaller have their gap
> diminished so that the line is long enough.
>
> Please review this at http://codereview.appspot.com/4609041/
>
> Affected files:
>  M scm/output-lib.scm
>
>
> Index: scm/output-lib.scm
> diff --git a/scm/output-lib.scm b/scm/output-lib.scm
> index
>
c25edf31f68a93de749a87e69e26cd4dde6dfc3d..dc2ff7a159313545f08fbfcb135a4a11b9f323fa
> 100644
> --- a/scm/output-lib.scm
> +++ b/scm/output-lib.scm
> @@ -915,13 +915,44 @@ between the two text elements."
>        (let* ((common (ly:grob-common-refpoint-of-array grob heads Y))
>               (head-down (ly:grob-array-ref heads 0))
>               (head-up (ly:grob-array-ref heads 1))
> -              (gap (ly:grob-property grob 'gap 0.35))
> +              ;; top of lower ambitus head:
> +              (ground (interval-end (ly:grob-extent head-down common Y)))
> +              ;; bottom of upper ambitus head:
> +              (roof (interval-start (ly:grob-extent head-up common Y)))
> +              ;; total amount of space between ambitus heads -
> +              ;; our task is to decide how much of this space will be
> occupied
> +              ;; by the ambitus line. We do this by calculating how big
> the gaps
> +              ;; between ambitus line and ambitus heads will be.
> +              (space-between-heads (- roof ground))
> +              ;; read the property - this is our starting point;
> +              ;; we are going to adjust it to fit small ambituses better:
> +              (gap-property (ly:grob-property grob 'gap 0.35))
> +              ;; We calculate a theoretical gap size using a linear
> function.
> +              ;; the function was determined by trial-and-error; it's main
> +              ;; premises are: gap is proportional to space between heads,
> +              ;; big value in gap property means that small ambituses
> won't
> +              ;; have any line at all, for values around 0.45 (default)
> +              ;; gap approaches 0 when distance betweeen heads approaches
> 0,
> +              ;; and it shouldn't reach 0 too soon with very small values.
> +              (proportional (+ (max gap-property 0.3) -0.45
> +                             (* 0.2 space-between-heads)))
> +              ;; ambitus line looks best if the gap value is quanted.
> +              ;; optimal quants are: 0.2 0.45 0.7 0.95 1.2 etc.
> +              (quanted-gap (+ (* (floor (/ (- proportional 0.2) 0.25))
> 0.25) 0.2))
> +              ;; we don't want to quant gap values smaller than 0.2
> +              ;; (because quanting them would mean making them 0).
> +              (theoretical (if (< proportional 0.2) proportional
> quanted-gap))
> +              ;; The above calculations are mainly for small ambituses;
> +              ;; in case of bigger ones they would lead to very big gaps
> +              ;; so we restrict them by the value written in the gap
> property.
> +              ;; we also don't want gap values too close to 0, hence the
> max.
> +              (gap (max (min theoretical gap-property) (/ gap-property
> 4.5)))
>               (point-min (+ (interval-end (ly:grob-extent head-down common
> Y))
>                             gap))
>               (point-max (- (interval-start (ly:grob-extent head-up common
> Y))
>                             gap)))
>
> -         (if (< point-min point-max)
> +         (if (< (+ point-min 0.1) point-max) ;; don't print lines shorter
> than 0.1
>              (let* ((layout (ly:grob-layout grob))
>                     (line-thick (ly:output-def-lookup layout
> 'line-thickness))
>                     (blot (ly:output-def-lookup layout 'blot-diameter))
>
>
>


-- 
Mike Subelsky
oib.com // ignitebaltimore.com // subelsky.com
@subelsky // (410) 929-4022

[Janek Warchol, 2011-06-15 07:03:34]

2011/6/13 Trevor Daniels <t.daniels@treda.co.uk>:
>
> Janek Warchoł wrote Monday, June 13, 2011 2:51 PM
>
>> 2011/6/13  <tdanielsmusic@googlemail.com>:
>
>>> a) It would be better to honour the value of 'gap if this
>>> is set by the user, rather than change a specifically
>>> requested gap value.
>>
>> My rationale is that it wouldn't make sense to set a big gap and
>> really want to have it applied to all ambituses.
>
> Your algorithm is fine as the default behaviour, but it does remove the
> ability for a user to precisely set the gap he wants by setting 'gap,
> for whatever reason.  This seems counter to Lily's flexible user control,
> but I don't feel too strongly about it.

Ok, let's give the user precise control.
I added a parameter which controlls this, but no reasonable name for it
(and for some intermediate stages in the code) comes to my mind.
Any suggestions to what should i change "woot" and "unwooted"?

Also, adding a new property means it has to be written somewhere -
i only found define-grobs.scm, but this is not enough: i get the following
warning when i compile

warning: cannot find property type-check for `woot' (backend-type?).
         perhaps a typing error?
warning: doing assignment anyway

Where else should i include it? I tried searching for "AmbitusLine",
but the only files i got (besides docs) were the two that i have.


>> Ofc i'm aware that this [quanting] is perhaps as nitpicky as it can go :D
>
> OK.  It is! :)   Does it scale correctly with large and small values of
> global staff-size?

Yes, the quanting stays the same.

cheers,
Janek

[karin.hoethker, 2011-06-16 22:53:35]

Hi Janek,

the description explains clearly how to use the parameters gap and woot. So, it
is a good starting point to understanding the scheme code that follows.

> I added a parameter which controlls this, but no reasonable name for it
> (and for some intermediate stages in the code) comes to my mind.
> Any suggestions to what should i change "woot" and "unwooted"?

What about "gap-adjustment-strength" or "gap-optimization-strength"?
- "gap" should be contained in the name, since that is the quantity changed by
woot
- "adjustment" because the gap is optimized
- "strength", because the value ranges between 0 and 1 (as opposed to boolean)

> 
> Yes, the quanting stays the same.

I couldn't find the verb "to quant" in a dictionary. Is it short for  "to
quantize"? If so, it might be clearer to change the description in
output-lib.scm.

Finally, I tested the gap adjustment for several intervals. For a fifth:

\new Staff \with { \consists Ambitus_engraver } {
    c' g'
}

the bottom of the ambitus line is very close to the lower note in the default
case. Is this on purpose? I would expect the ambitus line to be more or less
centered between note heads.

hope this helps,
Karin

[c_sorensen, 2011-06-16 23:25:29]

On 6/16/11 4:53 PM, "karin.hoethker@googlemail.com"
<karin.hoethker@googlemail.com> wrote:

> Hi Janek,
> 
> the description explains clearly how to use the parameters gap and woot.
> So, it is a good starting point to understanding the scheme code that
> follows.
> 
>> I added a parameter which controlls this, but no reasonable name for
> it
>> (and for some intermediate stages in the code) comes to my mind.
>> Any suggestions to what should i change "woot" and "unwooted"?
> 
> What about "gap-adjustment-strength" or "gap-optimization-strength"?
> - "gap" should be contained in the name, since that is the quantity
> changed by woot
> - "adjustment" because the gap is optimized
> - "strength", because the value ranges between 0 and 1 (as opposed to
> boolean)

Thanks for the ideas, Karin.  They triggered another one for me.

Can we use similar concepts to vertical spacing variables and call it
"gap-stretchability"?

Thanks,

Carl

[MikeSol, 2011-06-17 07:18:49]

Good work!  A few comments below.

http://codereview.appspot.com/4609041/diff/12001/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/4609041/diff/12001/scm/define-grobs.scm#newcode141
scm/define-grobs.scm:141: (woot . 1)
This seems like 1337 $p34k - I have never heard woot in any other context. 
Perhaps change to something more descriptive?

http://codereview.appspot.com/4609041/diff/12001/scm/output-lib.scm
File scm/output-lib.scm (right):

http://codereview.appspot.com/4609041/diff/12001/scm/output-lib.scm#newcode944
scm/output-lib.scm:944: (linear-gap (+ (max gap-property 0.3) -0.45
Indentation: the -0.45 should be on the next line & lined up with the
left-parenthesis of (max.

http://codereview.appspot.com/4609041/diff/12001/scm/output-lib.scm#newcode950
scm/output-lib.scm:950: (unwooted (max (min calculated-gap gap-property) (/
gap-property 4.5)))
80 column max

http://codereview.appspot.com/4609041/diff/12001/scm/output-lib.scm#newcode951
scm/output-lib.scm:951: (gap (+ (* unwooted woot) (* gap-property (- 1 woot))))
This codes a lot of properties.  I'm fine with the code (though I'd need to see
a regtest).  Can you try using a "details" property (like for the Beam grob)
that stores all of these values?

[Janek Warchol, 2011-06-21 13:04:07]

Hi Karin,

i'm back from my short vacation.

2011/6/17  <karin.hoethker@googlemail.com>:
> the description explains clearly how to use the parameters gap and woot.
> So, it is a good starting point to understanding the scheme code that
> follows.

Good!

>> Yes, the quanting stays the same.
>
> I couldn't find the verb "to quant" in a dictionary. Is it short for
> "to quantize"?

Yes. It's used a lot in beaming code.

> If so, it might be clearer to change the description in output-lib.scm.

Good point, it's better to avoid abbreviations.

> Finally, I tested the gap adjustment for several intervals. For a fifth:
>
> \new Staff \with { \consists Ambitus_engraver } {
>    c' g'
> }
>
> the bottom of the ambitus line is very close to the lower note in the
> default case. Is this on purpose? I would expect the ambitus line to be
> more or less centered between note heads.

Have you zoomed the output to check it? I suppose its a rasterization
problem; a lot of things seem to be wrong when output is watched
unzoomed on a computer screen (for example one stem in the attachment
looks two times thicker than the other, while in fact it's exactly the
same).

Thanks for review!
Janek

[Janek Warchol, 2011-06-21 13:04:38]

http://codereview.appspot.com/4609041/diff/12001/scm/define-grobs.scm
File scm/define-grobs.scm (right):

http://codereview.appspot.com/4609041/diff/12001/scm/define-grobs.scm#newcode141
scm/define-grobs.scm:141: (woot . 1)
On 2011/06/17 07:18:49, MikeSol wrote:
> This seems like 1337 $p34k -
> I have never heard woot in any other context. 
> Perhaps change to something more descriptive?

Of course!
I had no idea for the name and decided to use a placeholder and ask you instead
of wasting 15 minutes on something so simple (i was quite tired when i wrote
this code).

http://codereview.appspot.com/4609041/diff/12001/scm/output-lib.scm
File scm/output-lib.scm (right):

http://codereview.appspot.com/4609041/diff/12001/scm/output-lib.scm#newcode944
scm/output-lib.scm:944: (linear-gap (+ (max gap-property 0.3) -0.45
On 2011/06/17 07:18:49, MikeSol wrote:
> Indentation: the -0.45 should be on the next line & lined up with the
> left-parenthesis of (max.

Done.

http://codereview.appspot.com/4609041/diff/12001/scm/output-lib.scm#newcode950
scm/output-lib.scm:950: (unwooted (max (min calculated-gap gap-property) (/
gap-property 4.5)))
On 2011/06/17 07:18:49, MikeSol wrote:
> 80 column max

Done.

http://codereview.appspot.com/4609041/diff/12001/scm/output-lib.scm#newcode951
scm/output-lib.scm:951: (gap (+ (* unwooted woot) (* gap-property (- 1 woot))))
On 2011/06/17 07:18:49, MikeSol wrote:
> This codes a lot of properties.  I'm fine with the code (though I'd need to
see
> a regtest).  Can you try using a "details" property (like for the Beam grob)
> that stores all of these values?

Umm, I don't want to define properties like linear-gap, calculated-gap etc. They
are just temporary variables so that the code calculating final gap is easier to
read. Had i not used them, i would have to write everything explicitely like
this (with better indentation perhaps):

(gap 
  (+ 
    (* 
      (max 
        (min 
          (if 
            (< 
              (+ 
                (max (ly:grob-property grob 'gap 0.35) 0.3) 
                -0.45
                (* 
                  0.2 
                  (- 
                    (interval-start (ly:grob-extent head-up common Y))
                    (interval-end (ly:grob-extent head-down common Y))))) 
              0.2)
            (+ 
              (max (ly:grob-property grob 'gap 0.35) 0.3)
              -0.45
              (* 
                0.2 
                (- 
                  (interval-start (ly:grob-extent head-up common Y)) 
                  (interval-end (ly:grob-extent head-down common Y))))) 
            (+ 
              (* 
                (floor 
                  (/ 
                    (- 
                      (+ 
                        (max (ly:grob-property grob 'gap 0.35) 0.3) 
                        -0.45
                        (* 
                          0.2 
                          (- 
                            (interval-start (ly:grob-extent head-up common Y)) 
                            (interval-end (ly:grob-extent head-down common
Y))))) 
                      0.2) 
                    0.25))
                0.25) 
              0.2))
          (ly:grob-property grob 'gap 0.35)) 
        (/ (ly:grob-property grob 'gap 0.35) 4.5))
      (ly:grob-property grob 'woot 1))
    (* (ly:grob-property grob 'gap 0.35) 
       (- 1 (ly:grob-property grob 'woot 1)))))

looks suicidal...
When i noticed that point-max and point-min don't seem to be any properties but
only a sort of temporary variables, i used this idea for my piece of code. Maybe
i didn't understand how this works...

[mike_apollinemike.com, 2011-06-21 13:07:40]

On Jun 21, 2011, at 3:04 PM, lemniskata.bernoullego@gmail.com wrote:

> 
> http://codereview.appspot.com/4609041/diff/12001/scm/define-grobs.scm
> File scm/define-grobs.scm (right):
> 
>
http://codereview.appspot.com/4609041/diff/12001/scm/define-grobs.scm#newcode141
> scm/define-grobs.scm:141: (woot . 1)
> On 2011/06/17 07:18:49, MikeSol wrote:
>> This seems like 1337 $p34k -
>> I have never heard woot in any other context.
>> Perhaps change to something more descriptive?
> 
> Of course!
> I had no idea for the name and decided to use a placeholder and ask you
> instead of wasting 15 minutes on something so simple (i was quite tired
> when i wrote this code).
> 
> http://codereview.appspot.com/4609041/diff/12001/scm/output-lib.scm
> File scm/output-lib.scm (right):
> 
> http://codereview.appspot.com/4609041/diff/12001/scm/output-lib.scm#newcode944
> scm/output-lib.scm:944: (linear-gap (+ (max gap-property 0.3) -0.45
> On 2011/06/17 07:18:49, MikeSol wrote:
>> Indentation: the -0.45 should be on the next line & lined up with the
>> left-parenthesis of (max.
> 
> Done.
> 
> http://codereview.appspot.com/4609041/diff/12001/scm/output-lib.scm#newcode950
> scm/output-lib.scm:950: (unwooted (max (min calculated-gap gap-property)
> (/ gap-property 4.5)))
> On 2011/06/17 07:18:49, MikeSol wrote:
>> 80 column max
> 
> Done.
> 
> http://codereview.appspot.com/4609041/diff/12001/scm/output-lib.scm#newcode951
> scm/output-lib.scm:951: (gap (+ (* unwooted woot) (* gap-property (- 1
> woot))))
> On 2011/06/17 07:18:49, MikeSol wrote:
>> This codes a lot of properties.  I'm fine with the code (though I'd
> need to see
>> a regtest).  Can you try using a "details" property (like for the Beam
> grob)
>> that stores all of these values?
> 
> Umm, I don't want to define properties like linear-gap, calculated-gap
> etc. They are just temporary variables so that the code calculating
> final gap is easier to read. Had i not used them, i would have to write
> everything explicitely like this (with better indentation perhaps):
> 
> (gap
>  (+
>    (*
>      (max
>        (min
>          (if
>            (<
>              (+
>                (max (ly:grob-property grob 'gap 0.35) 0.3)
>                -0.45
>                (*
>                  0.2
>                  (-
>                    (interval-start (ly:grob-extent head-up common Y))
>                    (interval-end (ly:grob-extent head-down common
> Y)))))
>              0.2)
>            (+
>              (max (ly:grob-property grob 'gap 0.35) 0.3)
>              -0.45
>              (*
>                0.2
>                (-
>                  (interval-start (ly:grob-extent head-up common Y))
>                  (interval-end (ly:grob-extent head-down common Y)))))
>            (+
>              (*
>                (floor
>                  (/
>                    (-
>                      (+
>                        (max (ly:grob-property grob 'gap 0.35) 0.3)
>                        -0.45
>                        (*
>                          0.2
>                          (-
>                            (interval-start (ly:grob-extent head-up
> common Y))
>                            (interval-end (ly:grob-extent head-down
> common Y)))))
>                      0.2)
>                    0.25))
>                0.25)
>              0.2))
>          (ly:grob-property grob 'gap 0.35))
>        (/ (ly:grob-property grob 'gap 0.35) 4.5))
>      (ly:grob-property grob 'woot 1))
>    (* (ly:grob-property grob 'gap 0.35)
>       (- 1 (ly:grob-property grob 'woot 1)))))
> 
> looks suicidal...
> When i noticed that point-max and point-min don't seem to be any
> properties but only a sort of temporary variables, i used this idea for
> my piece of code. Maybe i didn't understand how this works...

What I meant is that every time you use a magic number (i.e. 0.35), consider
making it user-tweakable unless you are absolutely sure that there is no utility
in changing that number.

Cheers,
MS

[mail_philholmes.net, 2011-06-21 13:31:01]

----- Original Message ----- 
From: "Janek Warchoł" <lemniskata.bernoullego@gmail.com>
> Have you zoomed the output to check it? I suppose its a rasterization
> problem; a lot of things seem to be wrong when output is watched
> unzoomed on a computer screen (for example one stem in the attachment
> looks two times thicker than the other, while in fact it's exactly the
> same).


Don't forget that it's important to do this with the PDF output.  The way 
PDF viewers and the PNG generator convert the PDF to images does often cause 
the apparent oddities you're talking about.

--
Phil Holmes

[Janek Warchol, 2011-06-21 13:31:52]

2011/6/21 mike@apollinemike.com <mike@apollinemike.com>:
> What I meant is that every time you use a magic number (i.e. 0.35),
> consider making it user-tweakable unless you are absolutely sure
> that there is no utility in changing that number.

Ah, you meant this! :)
well, i think that 0.35 in (ly:grob-property grob 'gap 0.35) means "if
you have trouble reading gap property, use 0.35", so it's only kind of
default value.
As for other numbers, i'm sure that making them user-tweakable will be
overkill. They simply are coefficients in a function that does a very
nitpicky tweak; the function (as a whole) can be controlled by
gap-stretchability and i'm sure it's enough.
Personally i suppose that noone in the world - except me - will ever
care about gap-stretchability value, leave alone the coefficients in
the funciton :)

thanks,
Janek

[mike_apollinemike.com, 2011-06-21 14:05:47]

On Jun 21, 2011, at 3:31 PM, Janek Warchoł wrote:

> 2011/6/21 mike@apollinemike.com <mike@apollinemike.com>:
>> What I meant is that every time you use a magic number (i.e. 0.35),
>> consider making it user-tweakable unless you are absolutely sure
>> that there is no utility in changing that number.
> 
> Ah, you meant this! :)
> well, i think that 0.35 in (ly:grob-property grob 'gap 0.35) means "if
> you have trouble reading gap property, use 0.35", so it's only kind of
> default value.
> As for other numbers, i'm sure that making them user-tweakable will be
> overkill. They simply are coefficients in a function that does a very
> nitpicky tweak; the function (as a whole) can be controlled by
> gap-stretchability and i'm sure it's enough.
> Personally i suppose that noone in the world - except me - will ever
> care about gap-stretchability value, leave alone the coefficients in
> the funciton :)

Sorry sorry - I mis-mis-spoke (I should have read my previous post).  What I
meant is that these values could be wrapped up into a details property.  The
argument that user-tweakability is overkill isn't necessarily a bad thing -
check out the `details' property for the Slur and Beam grobs.  By creating a
similar details list, it'd be more LilyPond-esque, which makes the code easier
to read (and, on the off chance that someone actually wants to modify this
(which is admittedly rare), they'll be able to w/o having to change the source).

Cheers,
MS

[Neil Puttock, 2011-06-22 22:59:34]

Hi Janek,

I'd be much happier with this change if you used a callback for 'gap instead of
inserting new code into the print function.  That way it's easy for users to
override the default behaviour without adding more properties.

Cheers,
Neil

[Janek Warchol, 2013-08-29 13:14:39]

ambitus patch returns from the dead! Made simpler and use a callback.

[Keith, 2013-08-30 04:57:30]

LGTM except details

https://codereview.appspot.com/4609041/diff/29002/input/regression/ambitus-ga...
File input/regression/ambitus-gap.ly (right):

https://codereview.appspot.com/4609041/diff/29002/input/regression/ambitus-ga...
input/regression/ambitus-gap.ly:5: note heads are set by the @code{gap}
property. Also, ambitus line
"By default, @code{gap} is a function that reduces the gap for small intervals,
so line remains visible."

We cannot say "Also," because if I set the gap property to 0.4, then the line
will *not* be visible in fourth.

https://codereview.appspot.com/4609041/diff/29002/scm/output-lib.scm
File scm/output-lib.scm (right):

https://codereview.appspot.com/4609041/diff/29002/scm/output-lib.scm#newcode1252
scm/output-lib.scm:1252: (if (and (ly:grob-array? heads)
You should have an 'else' case (if <test> <body> <else>) so that if the test
ever fails you return some value; otherwise the user gets a mysterious error
message.

https://codereview.appspot.com/4609041/diff/29002/scm/output-lib.scm#newcode1258
scm/output-lib.scm:1258: (max-gap (ly:grob-property grob 'maximum-gap 2))
The defaults seem strange.  They should be typical values, otherwise people get
confused.  Is 'max-gap normally 2 ?

[Janek Warchol, 2013-08-30 10:00:39]

fix stuff found by Keith

[Janek Warchol, 2013-08-30 10:01:58]

https://codereview.appspot.com/4609041/diff/29002/input/regression/ambitus-ga...
File input/regression/ambitus-gap.ly (right):

https://codereview.appspot.com/4609041/diff/29002/input/regression/ambitus-ga...
input/regression/ambitus-gap.ly:5: note heads are set by the @code{gap}
property. Also, ambitus line
On 2013/08/30 04:57:30, Keith wrote:
> "By default, @code{gap} is a function that reduces the gap for small
intervals,
> so line remains visible."
> 
> We cannot say "Also," because if I set the gap property to 0.4, then the line
> will *not* be visible in fourth.

Done.

https://codereview.appspot.com/4609041/diff/29002/scm/output-lib.scm
File scm/output-lib.scm (right):

https://codereview.appspot.com/4609041/diff/29002/scm/output-lib.scm#newcode1252
scm/output-lib.scm:1252: (if (and (ly:grob-array? heads)
On 2013/08/30 04:57:30, Keith wrote:
> You should have an 'else' case (if <test> <body> <else>) so that if the test
> ever fails you return some value; otherwise the user gets a mysterious error
> message.

Done.

https://codereview.appspot.com/4609041/diff/29002/scm/output-lib.scm#newcode1258
scm/output-lib.scm:1258: (max-gap (ly:grob-property grob 'maximum-gap 2))
On 2013/08/30 04:57:30, Keith wrote:
> The defaults seem strange.  They should be typical values, otherwise people
get
> confused.  Is 'max-gap normally 2 ?

Good catch, this was a typo.  Done.

[Janek Warchol, 2013-09-01 16:43:08]

Just to let you know: i've tested the patch on a bunch of simple SATB scores and
the ambits look waaaaaaaaay better.

[Janek Warchol, 2013-09-06 07:53:47]

pushed as 1883fef6da1f91b1985ac296f0eb5bc9aee613ec