Changes the Y offset callback of multi-measure rest.

Changes the Y offset callback of multi-measure rest.

[MikeSol, 2011-04-11 23:11:01]

I was able to figure out a way to do this w/o an extra offset (I think). 
Please, read, test, & report back.

Cheers,
MS

[cpkc_shaw.ca, 2011-04-12 04:32:12]

On 11-04-11 05:11 PM, mtsolo@gmail.com wrote:
> Reviewers: ,
>
> Message:
> I was able to figure out a way to do this w/o an extra offset (I think).
>  Please, read, test, & report back.
>
> Cheers,
> MS

Mike, is this a new patch against issue 1604?  If at all possible, it 
helps uneducated bug squad folk a great deal when developers refer to 
numbered issues in their messages.

Colin

-- 
The test of our progress is not whether we add more to the abundance
of those who have much, it is whether we provide enough for those who
have too little.
-Franklin D. Roosevelt, 32nd US President (1882-1945)

[cpkc_shaw.ca, 2011-04-12 04:53:33]

On 11-04-11 05:11 PM, mtsolo@gmail.com wrote:
> Reviewers: ,
>
> Message:
> I was able to figure out a way to do this w/o an extra offset (I think).
>  Please, read, test, & report back.
>
> Cheers,
> MS
>
> Description:
> Changes the Y offset callback of multi-measure rest.
>
> Please review this at http://codereview.appspot.com/4389047/
>
> Affected files:
>   A input/regression/multi-measure-rest-y-offset.ly
>   M lily/multi-measure-rest.cc
>   M scm/define-grobs.scm
>   M scm/output-lib.scm
>
>
> Index: input/regression/multi-measure-rest-y-offset.ly
> diff --git a/input/regression/multi-measure-rest-y-offset.ly 
> b/input/regression/multi-measure-rest-y-offset.ly
> new file mode 100644
> index 
>
0000000000000000000000000000000000000000..480d5731cbdff96f1f8fafdb2b72a22c60393461
> --- /dev/null
> +++ b/input/regression/multi-measure-rest-y-offset.ly
> @@ -0,0 +1,15 @@
> +\version "2.13.60"
> +\header {
> +  texidoc = "The multi-measure rest's automatic vertical shifting can
> +be turned off with the automatic-shift property."
> +}
> +
> +\paper {
> +  ragged-right = ##t
> +}
> +
> +\relative c' {
> +  R1
> +  \override Staff . MultiMeasureRest #'Y-offset = #0
> +  R1
> +}
> \ No newline at end of file
> Index: lily/multi-measure-rest.cc
> diff --git a/lily/multi-measure-rest.cc b/lily/multi-measure-rest.cc
> index 
>
9628402a5ecec1ce899d4ef1d4ea57002b7529af..4e3a32f50c2fedec4f6795810cd4048ad3f10912

> 100644
> --- a/lily/multi-measure-rest.cc
> +++ b/lily/multi-measure-rest.cc
> @@ -137,8 +137,6 @@ Multi_measure_rest::symbol_stencil (Grob *me, Real 
> space)
>        return s;
>      }
>
> -  Real staff_space = Staff_symbol_referencer::staff_space (me);
> -
>    Font_metric *musfont = Font_interface::get_default_font (me);
>
>    SCM sml = me->get_property ("use-breve-rest");
> @@ -156,12 +154,6 @@ Multi_measure_rest::symbol_stencil (Grob *me, 
> Real space)
>      {
>        Stencil s = musfont->find_by_name (Rest::glyph_name (me, 0, "", 
> true));
>
> -      /*
> -        ugh.
> -      */
> -      if (Staff_symbol_referencer::get_position (me) == 0.0)
> -        s.translate_axis (staff_space, Y_AXIS);
> -
>        s.translate_axis ((space - s.extent (X_AXIS).length ()) / 2, 
> X_AXIS);
>
>        return s;
> Index: scm/define-grobs.scm
> diff --git a/scm/define-grobs.scm b/scm/define-grobs.scm
> index 
>
84c3f72f8fbb72c715ebab652bd81c2780b31998..a167659ed264a61402318c060e768bac38b2f7d9

> 100644
> --- a/scm/define-grobs.scm
> +++ b/scm/define-grobs.scm
> @@ -1309,7 +1309,7 @@
>      (staff-position . 0)
>      (stencil . ,ly:multi-measure-rest::print)
>      (thick-thickness . 6.6)
> -    (Y-offset . ,ly:staff-symbol-referencer::callback)
> +    (Y-offset . ,multi-measure-rest::callback)
>      (meta . ((class . Spanner)
>           (interfaces . (font-interface
>                  multi-measure-interface
> Index: scm/output-lib.scm
> diff --git a/scm/output-lib.scm b/scm/output-lib.scm
> index 
>
27c69fdfbe82eba10f1d3dcc20fe8156863c0926..1c2da4e7234c6835770934c74659d8dc7f876d3c

> 100644
> --- a/scm/output-lib.scm
> +++ b/scm/output-lib.scm
> @@ -780,6 +780,15 @@ between the two text elements."
>
>
>  ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
> +;; multi-measure-rest
> +
> +(define-public (multi-measure-rest::callback grob)
> +  (let ((y-offset (ly:staff-symbol-referencer::callback grob)))
> +    (if (and (eq? (ly:grob-property grob 'measure-count) 1) (eqv? 
> y-offset 0.0))
> +        (ly:staff-symbol-staff-space grob)
> +        y-offset)))
> +
> +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
>  ;; fret boards
>
>  (define-public (fret-board::calc-stencil grob)
>

Compiled cleanly and make check seemed OK, barring an assortment of midi 
logs and an almost unnoticeable shift in partcombine-midi.ly

When Mike confirms that this applies to issue 1604, I'll update the 
issue and set the patch status to "review".

Colin

-- 
The test of our progress is not whether we add more to the abundance
of those who have much, it is whether we provide enough for those who
have too little.
-Franklin D. Roosevelt, 32nd US President (1882-1945)

[mike_apollinemike.com, 2011-04-12 10:11:11]

On Apr 12, 2011, at 12:32 AM, Colin Campbell wrote:

> On 11-04-11 05:11 PM, mtsolo@gmail.com wrote:
>> Reviewers: ,
>> 
>> Message:
>> I was able to figure out a way to do this w/o an extra offset (I think).
>> Please, read, test, & report back.
>> 
>> Cheers,
>> MS
> 
> Mike, is this a new patch against issue 1604?  If at all possible, it helps
uneducated bug squad folk a great deal when developers refer to numbered issues
in their messages.
> 
> Colin

Yup!  It's a proposal for 1604.

Cheers,
MS

[mike_apollinemike.com, 2011-04-12 10:17:09]

On Apr 12, 2011, at 12:53 AM, Colin Campbell wrote:

> On 11-04-11 05:11 PM, mtsolo@gmail.com wrote:
>> Reviewers: ,
>> 
>> Message:
>> I was able to figure out a way to do this w/o an extra offset (I think).
>> Please, read, test, & report back.
>> 
>> Cheers,
>> MS
>> 
>> Description:
>> Changes the Y offset callback of multi-measure rest.
>> 
>> Please review this at http://codereview.appspot.com/4389047/
>> 
>> Affected files:
>>  A input/regression/multi-measure-rest-y-offset.ly
>>  M lily/multi-measure-rest.cc
>>  M scm/define-grobs.scm
>>  M scm/output-lib.scm
>> 
>> 
>> Index: input/regression/multi-measure-rest-y-offset.ly
>> diff --git a/input/regression/multi-measure-rest-y-offset.ly
b/input/regression/multi-measure-rest-y-offset.ly
>> new file mode 100644
>> index
0000000000000000000000000000000000000000..480d5731cbdff96f1f8fafdb2b72a22c60393461
>> --- /dev/null
>> +++ b/input/regression/multi-measure-rest-y-offset.ly
>> @@ -0,0 +1,15 @@
>> +\version "2.13.60"
>> +\header {
>> +  texidoc = "The multi-measure rest's automatic vertical shifting can
>> +be turned off with the automatic-shift property."
>> +}
>> +
>> +\paper {
>> +  ragged-right = ##t
>> +}
>> +
>> +\relative c' {
>> +  R1
>> +  \override Staff . MultiMeasureRest #'Y-offset = #0
>> +  R1
>> +}
>> \ No newline at end of file
>> Index: lily/multi-measure-rest.cc
>> diff --git a/lily/multi-measure-rest.cc b/lily/multi-measure-rest.cc
>> index
9628402a5ecec1ce899d4ef1d4ea57002b7529af..4e3a32f50c2fedec4f6795810cd4048ad3f10912
100644
>> --- a/lily/multi-measure-rest.cc
>> +++ b/lily/multi-measure-rest.cc
>> @@ -137,8 +137,6 @@ Multi_measure_rest::symbol_stencil (Grob *me, Real space)
>>       return s;
>>     }
>> 
>> -  Real staff_space = Staff_symbol_referencer::staff_space (me);
>> -
>>   Font_metric *musfont = Font_interface::get_default_font (me);
>> 
>>   SCM sml = me->get_property ("use-breve-rest");
>> @@ -156,12 +154,6 @@ Multi_measure_rest::symbol_stencil (Grob *me, Real
space)
>>     {
>>       Stencil s = musfont->find_by_name (Rest::glyph_name (me, 0, "", true));
>> 
>> -      /*
>> -        ugh.
>> -      */
>> -      if (Staff_symbol_referencer::get_position (me) == 0.0)
>> -        s.translate_axis (staff_space, Y_AXIS);
>> -
>>       s.translate_axis ((space - s.extent (X_AXIS).length ()) / 2, X_AXIS);
>> 
>>       return s;
>> Index: scm/define-grobs.scm
>> diff --git a/scm/define-grobs.scm b/scm/define-grobs.scm
>> index
84c3f72f8fbb72c715ebab652bd81c2780b31998..a167659ed264a61402318c060e768bac38b2f7d9
100644
>> --- a/scm/define-grobs.scm
>> +++ b/scm/define-grobs.scm
>> @@ -1309,7 +1309,7 @@
>>     (staff-position . 0)
>>     (stencil . ,ly:multi-measure-rest::print)
>>     (thick-thickness . 6.6)
>> -    (Y-offset . ,ly:staff-symbol-referencer::callback)
>> +    (Y-offset . ,multi-measure-rest::callback)
>>     (meta . ((class . Spanner)
>>          (interfaces . (font-interface
>>                 multi-measure-interface
>> Index: scm/output-lib.scm
>> diff --git a/scm/output-lib.scm b/scm/output-lib.scm
>> index
27c69fdfbe82eba10f1d3dcc20fe8156863c0926..1c2da4e7234c6835770934c74659d8dc7f876d3c
100644
>> --- a/scm/output-lib.scm
>> +++ b/scm/output-lib.scm
>> @@ -780,6 +780,15 @@ between the two text elements."
>> 
>> 
>> ;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
>> +;; multi-measure-rest
>> +
>> +(define-public (multi-measure-rest::callback grob)
>> +  (let ((y-offset (ly:staff-symbol-referencer::callback grob)))
>> +    (if (and (eq? (ly:grob-property grob 'measure-count) 1) (eqv? y-offset
0.0))
>> +        (ly:staff-symbol-staff-space grob)
>> +        y-offset)))
>> +
>> +;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;
>> ;; fret boards
>> 
>> (define-public (fret-board::calc-stencil grob)
>> 
> 
> Compiled cleanly and make check seemed OK, barring an assortment of midi logs
and an almost unnoticeable shift in partcombine-midi.ly
> 
> When Mike confirms that this applies to issue 1604, I'll update the issue and
set the patch status to "review".
> 
> Colin

I had similar problems with partcombine-midi.ly.  Colin - could you try
compiling partcombine-midi.ly against this patch manually and see if the same
problem arises?  When make check does it, I saw a shift, but upon manual
compilation, the shift disappeared.  Ditto for the files w/ log changes if
possible.  I'm not sure if it's a build system problem or a problem with the
current patch.

What's odd is that midi regtests are responding to this change whereas others
are not, given that this patch is a visual change that (in theory) has nothing
to do w/ MIDI output.

Cheers,
MS

[MikeSol, 2011-04-13 10:51:55]

The only (big) issue with this patch as it stands is that, at some point in the
chain, the Y-offset of the multi-measure-rest is being fed into that which
controls MultiMeasureRestText positioning.  Before, it would receive the
Y-offset of 0, which did not bump these texts (i.e. fermataMarkup) up.  Now, it
receives the new Y-offset, which causes these grobs to rise above their prior
placement.

I don't really where this bumping-up of MultiMeasureRestText grobs is
done...maybe Side_position_interface::general_side_position ?  Thoughts would be
appreciated!

Cheers,
Mike

[mike_apollinemike.com, 2011-04-13 12:05:27]

On Apr 13, 2011, at 6:51 AM, mtsolo@gmail.com wrote:

> The only (big) issue with this patch as it stands is that, at some point
> in the chain, the Y-offset of the multi-measure-rest is being fed into
> that which controls MultiMeasureRestText positioning.  Before, it would
> receive the Y-offset of 0, which did not bump these texts (i.e.
> fermataMarkup) up.  Now, it receives the new Y-offset, which causes
> these grobs to rise above their prior placement.
> 
> I don't really where this bumping-up of MultiMeasureRestText grobs is
> done...maybe Side_position_interface::general_side_position ?  Thoughts
> would be appreciated!
> 

After further reading, I get the sense that the issue comes from the fact that
the pure_relative_y_coordinate that an MMR needs to work is 0.0, and that the
shift of staff space can only happen after all of the positioning has been done
(read: in the print function).

It seems like the real problem is that
ly:side-position-interface::y-aligned-side is not the appropriate function for
MultiMeasureRestText #'Y-offset given how users likely want these grobs to be
typeset.  Try:

{
R1^\fermataMarkup
\override Staff . MultiMeasureRest #'Y-offset = #-1.5
R1^\fermataMarkup
\override Staff . MultiMeasureRest #'Y-offset = #0.5
R1^\fermataMarkup
\override Staff . MultiMeasureRest #'Y-offset = #1.0
R1^\fermataMarkup
\override Staff . MultiMeasureRest #'Y-offset = #2.0
R1^\fermataMarkup
}

IMO, the fermata should not move in any of these cases, or rather, its moving
should be a non-default behavior that can be requested by the user.

Assuming that others agree, I think that this should be fixed first before
tackling issue 1604.  And, if others agree, is there an existing function that
can achieve this behavior?

Cheers,
MS