Fattens the 256 first braces.

Fattens the 256 first braces.

[Carl, 2011-05-10 19:51:20]

LGTM, with a small nitpick.

I like the new braces better.

Carl

http://codereview.appspot.com/4518052/diff/1/mf/feta-braces.mf
File mf/feta-braces.mf (right):

http://codereview.appspot.com/4518052/diff/1/mf/feta-braces.mf#newcode148
mf/feta-braces.mf:148: fatten_factor := 1.5;
I think that good practice would have you do a save when you define a new
variable, so you won't override it if it's defined elsewhere in the file.  Using
save creates a local variable.

[Bertrand Bordage, 2011-05-10 20:08:03]

Thanks for the review !
Patch updated.
I don't know if there's a better way to solve this issue.
As big braces look good, I decided to limit the changes to the smallest ones.

[Trevor Daniels, 2011-05-10 22:53:55]

LGTM, apart from one nitpick.  I much prefer your
redesigned braces!

Trevor

http://codereview.appspot.com/4518052/diff/2002/mf/feta-braces.mf
File mf/feta-braces.mf (right):

http://codereview.appspot.com/4518052/diff/2002/mf/feta-braces.mf#newcode150
mf/feta-braces.mf:150: save fatten;
I don't think this placement of the save causes
any problems, but wouldn't be neater to place it
outside the loop with the other save commands?

[Bertrand Bordage, 2011-05-10 23:15:02]

http://codereview.appspot.com/4518052/diff/2002/mf/feta-braces.mf
File mf/feta-braces.mf (right):

http://codereview.appspot.com/4518052/diff/2002/mf/feta-braces.mf#newcode150
mf/feta-braces.mf:150: save fatten;
That's what I thought at first. Then I saw "save number;" line 129, so I decided
to put this here.

[dak, 2011-05-11 07:42:33]

If you say you fatten the first 256 braces, does that mean that there is a jump
in thickness when going from the 256th to the 257th brace?

In that case, would it not be better to have a smoother transition rather than a
jump?  Metafont is good at curves, after all...

[pacovila, 2011-05-11 08:01:28]

2011/5/11  <dak@gnu.org>:
> If you say you fatten the first 256 braces, does that mean that there is
> a jump in thickness when going from the 256th to the 257th brace?

Is it a joke?

-- 
Francisco Vila. Badajoz (Spain)
www.paconet.org , www.csmbadajoz.com

[pacovila, 2011-05-11 08:15:45]

2011/5/11 Francisco Vila <paconet.org@gmail.com>:
> 2011/5/11  <dak@gnu.org>:
>> If you say you fatten the first 256 braces, does that mean that there is
>> a jump in thickness when going from the 256th to the 257th brace?
>
> Is it a joke?

Oh, of course it is. Sorry :*)


-- 
Francisco Vila. Badajoz (Spain)
www.paconet.org , www.csmbadajoz.com

[Bertrand Bordage, 2011-05-11 08:49:00]

I don't know if this is a joke, so I answer to David's question :
of course there is no jump between the 256th and the 257th !
Refer to this message to see before/after :
http://lists.gnu.org/archive/html/lilypond-devel/2011-05/msg00150.html

[dak, 2011-05-11 08:54:40]

http://codereview.appspot.com/4518052/diff/2002/mf/feta-braces.mf
File mf/feta-braces.mf (right):

http://codereview.appspot.com/4518052/diff/2002/mf/feta-braces.mf#newcode151
mf/feta-braces.mf:151: fatten := 1 + fatten_factor - min ( number *
fatten_factor / 256, fatten_factor);
This is a bit obscure to me.  How about

    fatten := min(number/256,1)[1+fatten_factor,1];

That makes it more obvious that the factor is running from
1+fatten_factor to 1.

There does not seem to be much point in using min, either, regarding clarity. 
So maybe the whole thing can be written as

if number<256:
  x := x*(number/256[1+fatten_factor,1]);
fi

However, "fatten_factor" sounds like a _factor_, so it would make more sense to
change it to 2.5 (is that really the intended value?  Seems really large!) and
write this as

if number<256:
  x := (number/256)[x*fatten_factor,x];
fi

I think that expresses the meaning better, even without requiring an extra
variable "fatten".

[Bertrand Bordage, 2011-05-11 09:22:14]

http://codereview.appspot.com/4518052/diff/2002/mf/feta-braces.mf
File mf/feta-braces.mf (right):

http://codereview.appspot.com/4518052/diff/2002/mf/feta-braces.mf#newcode151
mf/feta-braces.mf:151: fatten := 1 + fatten_factor - min ( number *
fatten_factor / 256, fatten_factor);
Thanks !
Hum...
I agree with your idea of making this clearer.
I also agree that my factor isn't really a factor since it's neutral value is 0.
But the functions you gave do not produce the correct output.
And I can't manage to find something cleaner than what I wrote.
Also : we can't get rid of "fatten", as it is also used to thicken the ends of
the braces.

[dak, 2011-05-11 09:54:36]

On 2011/05/11 09:22:14, Bertrand Bordage wrote:
> http://codereview.appspot.com/4518052/diff/2002/mf/feta-braces.mf
> File mf/feta-braces.mf (right):
> 
> http://codereview.appspot.com/4518052/diff/2002/mf/feta-braces.mf#newcode151
> mf/feta-braces.mf:151: fatten := 1 + fatten_factor - min ( number *
> fatten_factor / 256, fatten_factor);
> Thanks !
> Hum...
> I agree with your idea of making this clearer.
> I also agree that my factor isn't really a factor since it's neutral value is
0.
> But the functions you gave do not produce the correct output.
> And I can't manage to find something cleaner than what I wrote.
> Also : we can't get rid of "fatten", as it is also used to thicken the ends of
> the braces.

Well, the middle formula was missing parens around number/256.  Other than that,
it should have worked fine.

Something like
fatten := min(number/256,1)[1+fatten_factor,1];
should do the trick then.

[dak, 2011-05-11 10:04:52]

On 2011/05/11 09:54:36, dak wrote:
> Something like
> fatten := min(number/256,1)[1+fatten_factor,1];
> should do the trick then.

Well, I repeat the suggestion to make fatten_factor neutral at 1 instead of 0
(resulting in omitting 1+ in the above formula), and perhaps instead of
hardcoding 256, one could make
it a parameter fatten_end or similar.

[Bertrand Bordage, 2011-05-11 10:14:11]

:)
Thanks ! This works great. Updated.

[Janek Warchol, 2011-05-11 11:32:15]

LGTM.
I like the results!