modifying default behaviour of tremolo slashes

modifying default behaviour of tremolo slashes

It turned out that tremolo slashes should have
quite constant slope (definately not depending
on beam slope), so i change that. I also changed
slash style so that it isn't rectangle in any
cases by dafault.

[Janek Warchol, 2011-07-04 19:26:04]

Passes regtests.

[Carl, 2011-07-04 19:50:09]

Looks mostly good to me.

Thanks,

Carl

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

http://codereview.appspot.com/4636081/diff/1/lily/stem-tremolo.cc#newcode42
lily/stem-tremolo.cc:42: that it's not proper notation.
We shouldn't keep the code in and commented out; let's just get rid of it.  No
old cruft kept around.

[Neil Puttock, 2011-07-04 20:11:48]

Hi Janek,

I'm afraid I don't like this at all.  While the authorities may be in agreement
that the slashes should be sloped, all the scores I've looked at follow the same
style as LilyPond (which I think reflects a more traditional hand-engraved
style).

It would be preferable to allow users to choose whether to keep the current
default or switch to the more modern behaviour.  This could be as simple as
adding a calc-slope callback which ignores beams and setting style to 'default.

Cheers,
Neil

[joeneeman, 2011-07-11 04:09:45]

Actually, non-rectangular, constant-sloped beams used to be the default, but I
changed them (some years ago now) to be rectangular and parallel to the beam,
since that's what most of my scores have. My music collection isn't with me
right now, but I can confirm at least that B&H's urtext edition of Bartok's solo
violin sonata uses rectangular, parallel-to-the-beam tremolos.

Cheers,
Joe

[Janek Warchol, 2011-07-17 07:25:43]

Finally - new patch set uploaded.
Now it's possible to easily switch between different tremolo behaviours.
'Style' property is no longer used to choose between rectangular and beam-like
slashes - this is now done using 'shape' property.
'Style' property now influences both 'shape' and 'slope' of the slashes:
- style=constant produces beam-like slashes with slope .4 in case of downstem
flagged notes and slope .25 in all other cases,
- style=varying is equal to current default behaviour.

It's possible to choose a combination of those by setting both 'style' and
'shape' properties.

Pdf demonstrating new behaviour:
http://lilypond.googlecode.com/issues/attachment?aid=17350003001&name=tremolo...

Please review. Suggestions on value names are most welcome!

[pkx166h, 2011-07-18 20:13:19]

Hello, can someone else verify?

I get a make fail:

--snip--

[/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-markup.scm]
Writing "internals.texi"...ERROR: In procedure procedure-name:
ERROR: Wrong type argument in position 1: #f
make[1]: *** [out/internals.texi] Error 1
rm out/weblinks.itexi
make[1]: Leaving directory `/home/jlowe/lilypond-git/build/Documentation'
make: *** [all] Error 2

[Janek Warchol, 2011-07-18 21:43:09]

2011/7/18  <pkx166h@gmail.com>:
> Hello, can someone else verify?
>
> I get a make fail:

I get it too... Strange, it compiled some time ago.
Btw,

>
[/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-markup.scm]
> Writing "internals.texi"...ERROR: In procedure procedure-name:
> ERROR: Wrong type argument in position 1: #f
> make[1]: *** [out/internals.texi] Error 1
> rm out/weblinks.itexi
> make[1]: Leaving directory
> `/home/jlowe/lilypond-git/build/Documentation'
> make: *** [all] Error 2

what does this mean?  I het similar errors from time to time and i
never know what to do with them.

cheers,
Janek

[Neil Puttock, 2011-07-18 21:51:43]

2011/7/18 Janek Warchoł <lemniskata.bernoullego@gmail.com>:

> Btw,
>
>>
[/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-markup.scm]
>> Writing "internals.texi"...ERROR: In procedure procedure-name:
>> ERROR: Wrong type argument in position 1: #f
>> make[1]: *** [out/internals.texi] Error 1
>> rm out/weblinks.itexi
>> make[1]: Leaving directory
>> `/home/jlowe/lilypond-git/build/Documentation'
>> make: *** [all] Error 2
>
> what does this mean?  I het similar errors from time to time and i
> never know what to do with them.

It usually means you've added a property but neglected to document it:

+++ b/scm/define-grobs.scm
@@ -1934,7 +1934,7 @@
(beam-width . ,ly:stem-tremolo::calc-width) ; staff-space
(slope . ,ly:stem-tremolo::calc-slope)
(stencil . ,ly:stem-tremolo::print)
- (style . ,ly:stem-tremolo::calc-style)
+ (shape . ,ly:stem-tremolo::calc-shape)

'shape needs documentation in scm/define-grob-properties.scm.

BTW, you're still reading 'style in stem-tremolo.cc, so it shouldn't
be removed from the interface macro.

Cheers,
Neil

[Janek Warchol, 2011-07-18 22:16:15]

2011/7/18 Neil Puttock <n.puttock@gmail.com>:
> 2011/7/18 Janek Warchoł <lemniskata.bernoullego@gmail.com>:
>
>> Btw,
>>
>>>
[/home/jlowe/lilypond-git/build/out/share/lilypond/current/scm/document-markup.scm]
>>> Writing "internals.texi"...ERROR: In procedure procedure-name:
>>> ERROR: Wrong type argument in position 1: #f
>>> make[1]: *** [out/internals.texi] Error 1
>>> rm out/weblinks.itexi
>>> make[1]: Leaving directory
>>> `/home/jlowe/lilypond-git/build/Documentation'
>>> make: *** [all] Error 2
>>
>> what does this mean?  I het similar errors from time to time and i
>> never know what to do with them.
>
> It usually means you've added a property but neglected to document it:
>
> +++ b/scm/define-grobs.scm
> @@ -1934,7 +1934,7 @@
> (beam-width . ,ly:stem-tremolo::calc-width) ; staff-space
> (slope . ,ly:stem-tremolo::calc-slope)
> (stencil . ,ly:stem-tremolo::print)
> - (style . ,ly:stem-tremolo::calc-style)
> + (shape . ,ly:stem-tremolo::calc-shape)
>
> 'shape needs documentation in scm/define-grob-properties.scm.
>
> BTW, you're still reading 'style in stem-tremolo.cc, so it shouldn't
> be removed from the interface macro.

Thanks!  New patch set uploaded to deal with this. (btw I hope that i
didn't write anything untrue there..,)  Makes from scratch.

cheers,
Janek

http://codereview.appspot.com/4636081/

[pkx166h, 2011-07-19 21:39:28]

Make works now and I get some reg test output differences

See

http://code.google.com/p/lilypond/issues/detail?id=1735#c4

James

[Janek Warchol, 2011-07-19 21:44:00]

2011/7/19  <pkx166h@gmail.com>:
> Make works now and I get some reg test output differences
> http://code.google.com/p/lilypond/issues/detail?id=1735#c4

They look exacly like they are expected to look.

thanks,
Janek

[joeneeman, 2011-07-21 08:39:53]

http://codereview.appspot.com/4636081/diff/10001/lily/stem-tremolo.cc
File lily/stem-tremolo.cc (right):

http://codereview.appspot.com/4636081/diff/10001/lily/stem-tremolo.cc#newcode42
lily/stem-tremolo.cc:42: style = ly_symbol2scm ("constant");
You can remove these two lines and use
style == ly_symbol2scm ("varying")
below

http://codereview.appspot.com/4636081/diff/10001/lily/stem-tremolo.cc#newcode93
lily/stem-tremolo.cc:93: style = ly_symbol2scm ("constant");
These two lines are redundant.

[Janek Warchol, 2011-07-23 19:58:19]

New patch set uploaded.  Current Lily default behaviour is kept as default now.

http://codereview.appspot.com/4636081/diff/10001/lily/stem-tremolo.cc
File lily/stem-tremolo.cc (right):

http://codereview.appspot.com/4636081/diff/10001/lily/stem-tremolo.cc#newcode42
lily/stem-tremolo.cc:42: style = ly_symbol2scm ("constant");
On 2011/07/21 08:39:53, joeneeman wrote:
> You can remove these two lines and use
> style == ly_symbol2scm ("varying")
> below

Done.

http://codereview.appspot.com/4636081/diff/10001/lily/stem-tremolo.cc#newcode93
lily/stem-tremolo.cc:93: style = ly_symbol2scm ("constant");
On 2011/07/21 08:39:53, joeneeman wrote:
> These two lines are redundant.

Isn't their purpose to guard against undefined (empty) style property?

[Janek Warchol, 2011-07-23 21:15:12]

http://codereview.appspot.com/4636081/diff/10001/lily/stem-tremolo.cc
File lily/stem-tremolo.cc (right):

http://codereview.appspot.com/4636081/diff/10001/lily/stem-tremolo.cc#newcode93
lily/stem-tremolo.cc:93: style = ly_symbol2scm ("constant");
On 2011/07/23 19:58:19, Janek Warchol wrote:
> On 2011/07/21 08:39:53, joeneeman wrote:
> > These two lines are redundant.
> 
> Isn't their purpose to guard against undefined (empty) style property?

(i've seen similar constructs in other places, that's why i used it)

[joeneeman, 2011-07-24 07:11:41]

http://codereview.appspot.com/4636081/diff/10001/lily/stem-tremolo.cc
File lily/stem-tremolo.cc (right):

http://codereview.appspot.com/4636081/diff/10001/lily/stem-tremolo.cc#newcode93
lily/stem-tremolo.cc:93: style = ly_symbol2scm ("constant");
On 2011/07/23 19:58:19, Janek Warchol wrote:
> On 2011/07/21 08:39:53, joeneeman wrote:
> > These two lines are redundant.
> 
> Isn't their purpose to guard against undefined (empty) style property?

If style is empty (or not a symbol) then (style == ly_symbol2scm ("varying"))
will be false and you'll just return 'default.

[pkx166h, 2011-07-24 10:49:34]

makes fine. No reg test differences now.

[Janek Warchol, 2011-07-25 00:08:17]

Joe: redundant lines removed.

Interesting thing happens when i compare regtests: i see a difference in
part-combine-tuplet-end.ly, which is completely unrelated to tremolos.  Even
funnier, my branch compiled this test better than master...
Perhaps i should've built from scratch.

BTW, should i add a regtest to this?

[Janek Warchol, 2011-07-29 05:38:26]

New patch set uploaded.  Minor style change: slash shape that was named
"default" (which was confusing) is now named "parallelogram" (because that's how
it looks like).
If you don't like parallelograms ;) we can name it beam-like or sth.

[reinhold_kainhofer.com, 2011-07-29 09:11:28]

Am Freitag 29 Juli 2011, 07:38:26 schrieben Sie:
> New patch set uploaded.  Minor style change: slash shape that was named
> "default" (which was confusing) is now named "parallelogram" (because
> that's how it looks like).
> If you don't like parallelograms ;) we can name it beam-like or sth.

Yes, please don't use "parallelogram"! After all, the slashes are 
parallelograms, too, so the name does not really distinguish the two styles...

Cheers,
Reinhold

-- 
------------------------------------------------------------------
Reinhold Kainhofer, Vienna University of Technology, Austria
email: reinhold@kainhofer.com, http://reinhold.kainhofer.com/
 * Financial and Actuarial Mathematics, TU Wien, http://www.fam.tuwien.ac.at/
 * Edition Kainhofer Music Publishing, http://www.edition-kainhofer.com/
 * LilyPond music typesetting software, http://www.lilypond.org/

[Janek Warchol, 2011-07-29 09:24:19]

Reinhold:
ok, replaced parallelogram with beam-like

[pkx166h, 2011-07-30 22:43:01]

make passes and no reg tests differences

[Graham Percival (old account), 2011-08-03 19:46:10]

cannot apply to master due to fixcc.

[Janek Warchol, 2011-08-03 22:08:24]

Oh no!  I hoped to get it on this countdown.
A version compatible with fixcc uploaded.

[Neil Puttock, 2011-08-11 20:05:10]

Needs a regression test exercising the different settings.

http://codereview.appspot.com/4636081/diff/38001/lily/stem-tremolo.cc
File lily/stem-tremolo.cc (right):

http://codereview.appspot.com/4636081/diff/38001/lily/stem-tremolo.cc#newcode118
lily/stem-tremolo.cc:118: shape = ly_symbol2scm ("beam-like");
this is redundant unless you want to add a warning for invalid shape setting
(you only check for rectangle below; anything else falls through to Lookup::beam
())

[Janek Warchol, 2011-08-22 10:11:55]

Regtest added.

http://codereview.appspot.com/4636081/diff/38001/lily/stem-tremolo.cc
File lily/stem-tremolo.cc (right):

http://codereview.appspot.com/4636081/diff/38001/lily/stem-tremolo.cc#newcode118
lily/stem-tremolo.cc:118: shape = ly_symbol2scm ("beam-like");
On 2011/08/11 20:05:10, Neil Puttock wrote:
> this is redundant unless you want to add a warning for invalid shape setting
> (you only check for rectangle below; anything else falls through to
Lookup::beam
> ())

True, but shouldn't we leave it to make the code more logical and elegant (and
in case we want to expand it in the future)?

[MikeSol, 2011-08-23 07:48:39]

LGTM

Cheers,
MS

http://codereview.appspot.com/4636081/diff/42001/lily/stem-tremolo.cc
File lily/stem-tremolo.cc (right):

http://codereview.appspot.com/4636081/diff/42001/lily/stem-tremolo.cc#newcode61
lily/stem-tremolo.cc:61: return scm_from_double ((Stem::duration_log (stem) >= 3
&& get_grob_direction (stem) == DOWN && !(beam))
!beam

http://codereview.appspot.com/4636081/diff/42001/lily/stem-tremolo.cc#newcode93
lily/stem-tremolo.cc:93: return ly_symbol2scm (((stemdir == UP && flag) || beam)
? "rectangle" : "beam-like");
return ly_symbol2scm (style != ly_symbol2scm ("constant") && ((stemdir == UP &&
flag) || beam) ? "rectangle" : "beam-like");

with proper indenting...

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

http://codereview.appspot.com/4636081/diff/42001/scm/define-grobs.scm#newcode...
scm/define-grobs.scm:1944: (X-extent . ,ly:stem-tremolo::width)
For consistency's sake (I just went through this with flags), keep this as a
style property and document the possible values in the Stem_tremolo docstring in
stem-tremolo.cc (the part with ADD_INTERFACE).

[janek, 2011-08-27 09:37:05]

New patch set uploaded.

http://codereview.appspot.com/4636081/diff/42001/lily/stem-tremolo.cc
File lily/stem-tremolo.cc (right):

http://codereview.appspot.com/4636081/diff/42001/lily/stem-tremolo.cc#newcode61
lily/stem-tremolo.cc:61: return scm_from_double ((Stem::duration_log (stem) >= 3
&& get_grob_direction (stem) == DOWN && !(beam))
On 2011/08/23 07:48:39, MikeSol wrote:
> !beam

Done.

http://codereview.appspot.com/4636081/diff/42001/lily/stem-tremolo.cc#newcode93
lily/stem-tremolo.cc:93: return ly_symbol2scm (((stemdir == UP && flag) || beam)
? "rectangle" : "beam-like");
On 2011/08/23 07:48:39, MikeSol wrote:
> return ly_symbol2scm (style != ly_symbol2scm ("constant") && ((stemdir == UP
&&
> flag) || beam) ? "rectangle" : "beam-like");
> 
> with proper indenting...

Done.

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

http://codereview.appspot.com/4636081/diff/42001/scm/define-grobs.scm#newcode...
scm/define-grobs.scm:1944: (X-extent . ,ly:stem-tremolo::width)
On 2011/08/23 07:48:39, MikeSol wrote:
> For consistency's sake (I just went through this with flags), keep this as a
> style property and document the possible values in the Stem_tremolo docstring
in
> stem-tremolo.cc (the part with ADD_INTERFACE).

I'm not sure if i understand.  Do you say that i should rename shape to style?

[pkx166h, 2011-08-28 11:00:39]

Still passes make and reg tests

[janek, 2011-09-05 20:31:48]

Hey Mike,

2011/8/27  <janek.lilypond@gmail.com>:
>
http://codereview.appspot.com/4636081/diff/42001/scm/define-grobs.scm#newcode...
> scm/define-grobs.scm:1944: (X-extent . ,ly:stem-tremolo::width)
> On 2011/08/23 07:48:39, MikeSol wrote:
>>
>> For consistency's sake (I just went through this with flags), keep this
>> as a style property and document the possible values in the Stem_tremolo
>> docstring in stem-tremolo.cc (the part with ADD_INTERFACE).
>
> I'm not sure if i understand.  Do you say that i should rename shape to
> style?

I'm not sure whether i should push this patch or not.  Can you explain
what is your concern?

cheers,
Janek

http://codereview.appspot.com/4636081/

[mike_apollinemike.com, 2011-09-16 16:03:40]

On Sep 5, 2011, at 10:31 PM, Janek Warchoł wrote:

> Hey Mike,
> 
> 2011/8/27  <janek.lilypond@gmail.com>:
>>
http://codereview.appspot.com/4636081/diff/42001/scm/define-grobs.scm#newcode...
>> scm/define-grobs.scm:1944: (X-extent . ,ly:stem-tremolo::width)
>> On 2011/08/23 07:48:39, MikeSol wrote:
>>> 
>>> For consistency's sake (I just went through this with flags), keep this
>>> as a style property and document the possible values in the Stem_tremolo
>>> docstring in stem-tremolo.cc (the part with ADD_INTERFACE).
>> 
>> I'm not sure if i understand.  Do you say that i should rename shape to
>> style?
> 
> I'm not sure whether i should push this patch or not.  Can you explain
> what is your concern?
> 
> cheers,
> Janek
> 

Sorry - I meant that the property name should be `style.'  Could you send me a
quick update stating where you're at w/ this patch?

Cheers,
MS

[janek, 2011-09-17 19:29:40]

2011/9/16 mike@apollinemike.com <mike@apollinemike.com>:
> On Sep 5, 2011, at 10:31 PM, Janek Warchoł wrote:
>
>> Hey Mike,
>>
>> 2011/8/27  <janek.lilypond@gmail.com>:
>>>
http://codereview.appspot.com/4636081/diff/42001/scm/define-grobs.scm#newcode...
>>> scm/define-grobs.scm:1944: (X-extent . ,ly:stem-tremolo::width)
>>> On 2011/08/23 07:48:39, MikeSol wrote:
>>>>
>>>> For consistency's sake (I just went through this with flags), keep this
>>>> as a style property and document the possible values in the Stem_tremolo
>>>> docstring in stem-tremolo.cc (the part with ADD_INTERFACE).
>>>
>>> I'm not sure if i understand.  Do you say that i should rename shape to
>>> style?
>>
>> I'm not sure whether i should push this patch or not.  Can you explain
>> what is your concern?
>
> Sorry - I meant that the property name should be `style.'

But there is 'style' property, only it's used for something else.
Before my patch, there was 'style' property which was about the
slashes being rectangular or not.  My patch renames this to 'shape'
property and intoduces 'style' property that does something else.
So, if we keep 'style' property doing what it was doing already, how
should we name the new property?
And BTW, maybe i forgot to add something somewhere?  Maybe it should
be listed in define-grobs.scm?

> Could you send me a quick update stating where you're at w/ this patch?

It is ready to go except for the above.

I hope this is clear now.

cheers,
Janek

[Valentin Villenave, 2015-04-12 20:25:32]

On 2011/09/17 19:29:40, janek wrote:
> It is ready to go except for the above.

Greetings,
I realize that this patch has been sort of forgotten, so I thought I’d update it
and re-upload it:
https://codereview.appspot.com/227070043/