improving the transition between full-length and shortened stems.

improving the transition between full-length and shortened stems.

Now the change in length between regular stems (3.5 ss) and shortened stems (2.5 ss) is executed more gradually.

more flexible transition between regular and shortened stems

(BUGGED) Transition in stem length depends on staff radius and other things

cosmetic bugfix

Some bugs fixed with transition length

Combined commit: smooth transition between regular and shortened stems.

This is first solution with Han-Wen suggestions added.

[Janek Warchol, 2011-02-24 16:47:27]

This patch handles transition between full length and shortened stems. It
follows Han-Wen suggestions and supports custom stem length and custom staves.

proof-sheets:
Lily 2.13.47 output - http://www.sendspace.com/file/7fy9i4
this patch output - http://www.sendspace.com/file/tgmlfc
source code: http://www.sendspace.com/file/3pusct

[Graham Percival, 2011-02-25 05:32:15]

On 2/24/11, lemniskata.bernoullego@gmail.com
<lemniskata.bernoullego@gmail.com> wrote:
> This patch handles transition between full length and shortened stems.
> It follows Han-Wen suggestions and supports custom stem length and
> custom staves.

Thanks, this now:
http://code.google.com/p/lilypond/issues/detail?id=1538

Cheers,
- Graham

[hanwenn, 2011-02-25 14:53:01]

LGTM

just cosmetics.

http://codereview.appspot.com/4210051/diff/1/lily/stem.cc
File lily/stem.cc (right):

http://codereview.appspot.com/4210051/diff/1/lily/stem.cc#newcode336
lily/stem.cc:336: SCM s = ly_assoc_get (ly_symbol2scm ("lengths"), details,
SCM_EOL);
can you rename this for easier readability?

http://codereview.appspot.com/4210051/diff/1/lily/stem.cc#newcode360
lily/stem.cc:360: Real shortening_step = min (max (0.25, (shorten_property /
6)), 0.5);
shouldnt 6 be 2*staff_radius+1 ?

http://codereview.appspot.com/4210051/diff/1/lily/stem.cc#newcode368
lily/stem.cc:368: which_step += abs(hp[dir]);
can you collapse these 3 lines?

[Janek Warchol, 2011-02-26 22:41:06]

2011/2/25 <hanwenn@gmail.com>

> LGTM
>
> just cosmetics.
>
>
> http://codereview.appspot.com/4210051/diff/1/lily/stem.cc
> File lily/stem.cc (right):
>
> http://codereview.appspot.com/4210051/diff/1/lily/stem.cc#newcode336
> lily/stem.cc:336: SCM s = ly_assoc_get (ly_symbol2scm ("lengths"),
> details, SCM_EOL);
> can you rename this for easier readability?
>

ok

http://codereview.appspot.com/4210051/diff/1/lily/stem.cc#newcode360
> lily/stem.cc:360: Real shortening_step = min (max (0.25,
> (shorten_property / 6)), 0.5);
> shouldnt 6 be 2*staff_radius+1 ?
>

No, radius of default staff is 2, so 2*staff_radius+1 = 5 != 6


> http://codereview.appspot.com/4210051/diff/1/lily/stem.cc#newcode368
> lily/stem.cc:368: which_step += abs(hp[dir]);
> can you collapse these 3 lines?


I thought it will be easier to understand this way, but no problem.
I'll fix these probably on wednesday - unfortunately i haven't configured my
home computer properly and i cannot access it via remote desktop :( Sorry
for the delay.

cheers,
Janek

[hanwenn, 2011-02-26 23:43:37]

2011/2/26 Janek Warchoł <lemniskata.bernoullego@gmail.com>:

http://codereview.appspot.com/4210051/diff/1/lily/stem.cc#newcode360
>> lily/stem.cc:360: Real shortening_step = min (max (0.25,
>> (shorten_property / 6)), 0.5);
>> shouldnt 6 be 2*staff_radius+1 ?
>
> No, radius of default staff is 2, so 2*staff_radius+1 = 5 != 6

well, 2*radius+2 then?  Anything that indicates how this number is formed is ok.

-- 
Han-Wen Nienhuys - hanwen@xs4all.nl - http://www.xs4all.nl/~hanwen

[Janek Warchol, 2011-02-27 07:29:22]

2011/2/27 Han-Wen Nienhuys <hanwenn@gmail.com>
>
> 2011/2/26 Janek Warchoł <lemniskata.bernoullego@gmail.com>:
>
> http://codereview.appspot.com/4210051/diff/1/lily/stem.cc#newcode360
> >> lily/stem.cc:360: Real shortening_step = min (max (0.25,
> >> (shorten_property / 6)), 0.5);
> >> shouldnt 6 be 2*staff_radius+1 ?
> >
> > No, radius of default staff is 2, so 2*staff_radius+1 = 5 != 6
>
> well, 2*radius+2 then?  Anything that indicates how this number is formed is
ok.

Ah, sorry. I missed what you meant.
Well, this nuber is not formed :) It shouldn't depend on staff_radius.
This number determines what the steepness of transition should be
(i.e., what the angle of the red line in the attachment will be). This
angle shouldn't depend on Staff_radius in my opinion.

cheers,
Janek

[wl_gnu.org, 2011-02-27 07:39:02]

>> well, 2*radius+2 then?  Anything that indicates how this number is
>> formed is ok.
> 
> Ah, sorry. I missed what you meant.
> Well, this nuber is not formed :) It shouldn't depend on
> staff_radius.  This number determines what the steepness of
> transition should be (i.e., what the angle of the red line in the
> attachment will be). This angle shouldn't depend on Staff_radius in
> my opinion.

Then add this comment to the sources and mark the value as heuristic.


    Werner

[Graham Percival (old account), 2011-02-27 08:46:09]

On 2011/02/27 07:39:02, wl_gnu.org wrote:
> > Well, this nuber is not formed :) It shouldn't depend on
> > staff_radius.  This number determines what the steepness of
> > transition should be (i.e., what the angle of the red line in the
> > attachment will be). This angle shouldn't depend on Staff_radius in
> > my opinion.
> 
> Then add this comment to the sources and mark the value as heuristic.

Once you've done that, could you send me the final patch so that I can push it?

Cheers,
- Graham

[Janek Warchol, 2011-02-27 23:42:46]

2011/2/27  <percival.music.ca@gmail.com>:
> On 2011/02/27 07:39:02, wl_gnu.org wrote:
>>
>> > Well, this nuber is not formed :) It shouldn't depend on
>> > staff_radius.  This number determines what the steepness of
>> > transition should be (i.e., what the angle of the red line in the
>> > attachment will be). This angle shouldn't depend on Staff_radius in
>> > my opinion.
>
>> Then add this comment to the sources and mark the value as heuristic.

Done, along with previous suggestions.

> Once you've done that, could you send me the final patch so that I can
> push it?

Here you are. This contains all the changes and should apply cleanly
to origin/master.
cheers,
Janek

[Graham Percival, 2011-02-28 07:39:20]

On 2/27/11, Janek Warchoł <lemniskata.bernoullego@gmail.com> wrote:
> Here you are. This contains all the changes and should apply cleanly
> to origin/master.

Thanks, pushed.

Cheers,
- Graham

[t.daniels_treda.co.uk, 2011-02-28 12:22:42]

Graham Percival wrote Monday, February 28, 2011 7:39 AM


> On 2/27/11, Janek Warchoł <lemniskata.bernoullego@gmail.com> 
> wrote:
>> Here you are. This contains all the changes and should apply 
>> cleanly
>> to origin/master.
>
> Thanks, pushed.

This commit causes many changes to the reg tests due to the small
increase in some of the stems that protrude beyond the staff.
Virtually all of the changes look good, but one struck me as 
undesirable.

It is in cue-clef-begin-of-score.ly.  Here the stems on the d 
full-size
notes seem undesirably lengthened.  Is the clef change being
ignored here?

Trevor 

[Janek Warchol, 2011-02-28 13:02:16]

2011/2/28 Trevor Daniels <t.daniels@treda.co.uk>
>
> Graham Percival wrote Monday, February 28, 2011 7:39 AM
>>
>> On 2/27/11, Janek Warchoł <lemniskata.bernoullego@gmail.com> wrote:
>>>
>>> Here you are. This contains all the changes and should apply cleanly
>>> to origin/master.
>>
>> Thanks, pushed.
>
> This commit causes many changes to the reg tests due to the small
> increase in some of the stems that protrude beyond the staff.
> Virtually all of the changes look good, but one struck me as undesirable.


I should've checked this myself... Sorry for not doing this.

> It is in cue-clef-begin-of-score.ly.  Here the stems on the d full-size
> notes seem undesirably lengthened.  Is the clef change being
> ignored here?

I don't have access to my main machine so i cannot see new output for myself,
but i can say that in this regtest all four notes should have longer stems now.
In general, stems of the notes that are on the middle line will be
longer now and it is desired behaviour. (until now it was too short,
see attachment).

cheers,
Janek

[t.daniels_treda.co.uk, 2011-03-01 12:41:55]

Janek Warchoł wrote Monday, February 28, 2011 1:02 PM


> 2011/2/28 Trevor Daniels <t.daniels@treda.co.uk>
>
>> This commit causes many changes to the reg tests due to the small
>> increase in some of the stems that protrude beyond the staff.
>> Virtually all of the changes look good, but one struck me as 
>> undesirable.
>>
>> It is in cue-clef-begin-of-score.ly. Here the stems on the d 
>> full-size
>> notes seem undesirably lengthened. Is the clef change being
>> ignored here?
>
> I don't have access to my main machine so i cannot see new output 
> for myself,
> but i can say that in this regtest all four notes should have 
> longer stems now.
> In general, stems of the notes that are on the middle line will be
> longer now and it is desired behaviour.

Yes, they are longer, but the stems on the full-size notes look too 
long
in this context to my eye.

I've attached the output from 2.13.52.

Trevor

[Janek Warchol, 2011-03-02 20:53:25]

2011/3/1 Trevor Daniels <t.daniels@treda.co.uk>
>
> Janek Warchoł wrote Monday, February 28, 2011 1:02 PM
>
>> 2011/2/28 Trevor Daniels <t.daniels@treda.co.uk>
>>
>>> This commit causes many changes to the reg tests due to the small
>>> increase in some of the stems that protrude beyond the staff.
>>> Virtually all of the changes look good, but one struck me as undesirable.
>>>
>>> It is in cue-clef-begin-of-score.ly. Here the stems on the d full-size
>>> notes seem undesirably lengthened. Is the clef change being
>>> ignored here?
>>
>> I don't have access to my main machine so i cannot see new output for myself,
>> but i can say that in this regtest all four notes should have longer stems
now.
>> In general, stems of the notes that are on the middle line will be
>> longer now and it is desired behaviour.
>
> Yes, they are longer, but the stems on the full-size notes look too long
> in this context to my eye.
>
> I've attached the output from 2.13.52.

In my opinion it is ok.
While i agree that in this particular example the stems may look
*slightly* better if they were a bit shorter, i suppose that
implementing this (i.e. making stem length depend on a broader context
than their position on the staff) would require too much work; i also
don't see by what criteria this could be judged. Since vast majority
of cases look better with this patch, and this problem isn't serious
imo, i suggest leaving it alone.

cheers,
Janek

PS my first patch is now in an official LilyPond release! Hooray!

[t.daniels_treda.co.uk, 2011-03-02 23:52:14]

Janek Warchoł wrote Wednesday, March 02, 2011 8:53 PM


> 2011/3/1 Trevor Daniels <t.daniels@treda.co.uk>
>
>> Yes, they are longer, but the stems on the full-size notes look 
>> too long
>> in this context to my eye.
>>
>> I've attached the output from 2.13.52.
>
> In my opinion it is ok.

It's OK; just not optimum when isolated like this.

> While i agree that in this particular example the stems may look
> *slightly* better if they were a bit shorter, i suppose that
> implementing this (i.e. making stem length depend on a broader 
> context
> than their position on the staff) would require too much work; i 
> also
> don't see by what criteria this could be judged. Since vast 
> majority
> of cases look better with this patch, and this problem isn't 
> serious
> imo, i suggest leaving it alone.

On balance your patch is a definite improvement, so I agree.

> PS my first patch is now in an official LilyPond release! Hooray!

Congratulations!  Welcome aboard!

Trevor