Re: Create engravers for merging rests (issue 321930043 by horndude77@gmail.com)

classic Classic list List threaded Threaded
14 messages Options
Reply | Threaded
Open this post in threaded view
|

Re: Create engravers for merging rests (issue 321930043 by horndude77@gmail.com)

pkx166h
Passes make, make check and a full make doc.

https://codereview.appspot.com/321930043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Create engravers for merging rests (issue 321930043 by horndude77@gmail.com)

David Nalesnik

https://codereview.appspot.com/321930043/diff/60001/ly/init.ly
File ly/init.ly (right):

https://codereview.appspot.com/321930043/diff/60001/ly/init.ly#newcode36
ly/init.ly:36: #(use-modules (scm merge-rests-engraver))
I'm not sure why you are defining a separate module.  The usual
procedure would be to add your functionality to an existing file in the
load list or add your new file to the load list (in scm/lily.scm -- see
init-scheme-files-body).

The Span_stem_engraver, for example, puts the bulk of its code in
scm/music-functions-init.scm.  (There is also some code in
scm.scheme-engravers.scm -- I'm not sure if you ought to add something
there.)

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm
File scm/merge-rests-engraver.scm (right):

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode14
scm/merge-rests-engraver.scm:14: (eq?
Here (and elsewhere) use eqv? to compare numbers.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode23
scm/merge-rests-engraver.scm:23: (define-public merge-rests-engraver
Here (and below) use the scheme-engraver macro for consistency.  As far
as I'm aware, all Scheme engravers in the code base use this now.  See
scm/scheme-engravers.scm or input/regression/scheme-text-spanner.ly for
examples.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode35
scm/merge-rests-engraver.scm:35: (if (eq? 'Rest (assoc-ref
(ly:grob-property grob 'meta) 'name))
(See comment about recognizing grobs below.)

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode39
scm/merge-rests-engraver.scm:39: (eq?
eqv?

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode44
scm/merge-rests-engraver.scm:44: (eq? (ly:grob-property mmrest
'measure-count) 1))
eqv?

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode70
scm/merge-rests-engraver.scm:70: (let* ((curr-rests '())
let* not needed -- use let

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode81
scm/merge-rests-engraver.scm:81: (if (eq? 'MultiMeasureRest (assoc-ref
(ly:grob-property grob 'meta) 'name))
You could use grob::name here.  Ordinarily, though, objects are
recognized by their interfaces.  So, here you should try

(if (grob::has-interface grob 'multi-measure-rest-interface)
   [...]

https://codereview.appspot.com/321930043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Create engravers for merging rests (issue 321930043 by horndude77@gmail.com)

Thomas Morley-2
In reply to this post by pkx166h

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm
File scm/merge-rests-engraver.scm (right):

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode10
scm/merge-rests-engraver.scm:10: (define (rest-length rest)
This definition is unused later and wouldn't work because of here
undefined 'rest-a'.
Maybe change it to something iterating over a list, comparing their
elements looking at their 'duration-log for Rests and 'measure-count for
MultiMeasureRests.
And use it for checking equal Rests/MMRs.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode23
scm/merge-rests-engraver.scm:23: (define-public merge-rests-engraver
Two general questions:
(1)
Is it possible to merge both engravers or is there a use case to have
them separated?
(2)
What do you think about introducing a property to switch rest-merging
on/off.
Could be a grob-property, both support rest-interface. Or probably a
context-property because the engraver(s) are in Staff.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode72
scm/merge-rests-engraver.scm:72: `((start-translation-timestep .
,(lambda (trans)
The order:
start-translation-timestep
stop-translation-timestep
finalize
acknowledgers
feels irritating and does not correspondend to the time they are called.
Any reason I for it, I'm not aware?

https://codereview.appspot.com/321930043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Create engravers for merging rests (issue 321930043 by horndude77@gmail.com)

Jay Anderson
In reply to this post by pkx166h

https://codereview.appspot.com/321930043/diff/60001/ly/init.ly
File ly/init.ly (right):

https://codereview.appspot.com/321930043/diff/60001/ly/init.ly#newcode36
ly/init.ly:36: #(use-modules (scm merge-rests-engraver))
On 2017/05/18 14:15:23, david.nalesnik wrote:
> I'm not sure why you are defining a separate module.  The usual
procedure would
> be to add your functionality to an existing file in the load list or
add your
> new file to the load list (in scm/lily.scm -- see
init-scheme-files-body).

> The Span_stem_engraver, for example, puts the bulk of its code in
> scm/music-functions-init.scm.  (There is also some code in
> scm.scheme-engravers.scm -- I'm not sure if you ought to add something
there.)

Done.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm
File scm/merge-rests-engraver.scm (right):

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode10
scm/merge-rests-engraver.scm:10: (define (rest-length rest)
On 2017/05/18 20:54:41, thomasmorley651 wrote:
> This definition is unused later and wouldn't work because of here
undefined
> 'rest-a'.
> Maybe change it to something iterating over a list, comparing their
elements
> looking at their 'duration-log for Rests and 'measure-count for
> MultiMeasureRests.
> And use it for checking equal Rests/MMRs.

I removed the method, though you're right I should investigate removing
that duplication.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode14
scm/merge-rests-engraver.scm:14: (eq?
On 2017/05/18 14:15:24, david.nalesnik wrote:
> Here (and elsewhere) use eqv? to compare numbers.

Done.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode23
scm/merge-rests-engraver.scm:23: (define-public merge-rests-engraver
On 2017/05/18 14:15:24, david.nalesnik wrote:
> Here (and below) use the scheme-engraver macro for consistency.  As
far as I'm
> aware, all Scheme engravers in the code base use this now.  See
> scm/scheme-engravers.scm or input/regression/scheme-text-spanner.ly
for
> examples.

Done.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode23
scm/merge-rests-engraver.scm:23: (define-public merge-rests-engraver
On 2017/05/18 20:54:41, thomasmorley651 wrote:
> Two general questions:
> (1)
> Is it possible to merge both engravers or is there a use case to have
them
> separated?

Done. They're not combined.

> (2)
> What do you think about introducing a property to switch rest-merging
on/off.
> Could be a grob-property, both support rest-interface. Or probably a
> context-property because the engraver(s) are in Staff.

Very good point. I haven't done this yet, but it should be part of this.
Will investigate.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode35
scm/merge-rests-engraver.scm:35: (if (eq? 'Rest (assoc-ref
(ly:grob-property grob 'meta) 'name))
On 2017/05/18 14:15:23, david.nalesnik wrote:
> (See comment about recognizing grobs below.)

Done.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode39
scm/merge-rests-engraver.scm:39: (eq?
On 2017/05/18 14:15:24, david.nalesnik wrote:
> eqv?

Done.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode44
scm/merge-rests-engraver.scm:44: (eq? (ly:grob-property mmrest
'measure-count) 1))
On 2017/05/18 14:15:24, david.nalesnik wrote:
> eqv?

Done.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode70
scm/merge-rests-engraver.scm:70: (let* ((curr-rests '())
On 2017/05/18 14:15:24, david.nalesnik wrote:
> let* not needed -- use let

Done.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode72
scm/merge-rests-engraver.scm:72: `((start-translation-timestep .
,(lambda (trans)
On 2017/05/18 20:54:41, thomasmorley651 wrote:
> The order:
> start-translation-timestep
> stop-translation-timestep
> finalize
> acknowledgers
> feels irritating and does not correspondend to the time they are
called.
> Any reason I for it, I'm not aware?

Done.

https://codereview.appspot.com/321930043/diff/60001/scm/merge-rests-engraver.scm#newcode81
scm/merge-rests-engraver.scm:81: (if (eq? 'MultiMeasureRest (assoc-ref
(ly:grob-property grob 'meta) 'name))
On 2017/05/18 14:15:23, david.nalesnik wrote:
> You could use grob::name here.  Ordinarily, though, objects are
recognized by
> their interfaces.  So, here you should try

> (if (grob::has-interface grob 'multi-measure-rest-interface)
>    [...]

Since both multimeasure rests and rests have the rest-interface further
checking is required. Take a look at the latest change and let me know
if I should check for the multi-measure-rest-interface instead.

https://codereview.appspot.com/321930043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Create engravers for merging rests (issue 321930043 by horndude77@gmail.com)

Thomas Morley-2
In reply to this post by pkx166h
Much better now, though:


https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm
File scm/scheme-engravers.scm (right):

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm#newcode151
scm/scheme-engravers.scm:151: (define (rest-eqv rest-len-prop)
The current patch-set fails with:
"Variable used before given a value: rest-eqv"
because subsequent usage of 'define ...' is equivalent to let not let*

Moving (define (rest-eqv rest-len-prop) before the engraver-definiton
starts will work or something at the lines of

...
   (let* ((rest-eqv
            (lambda (rest-len-prop)
              (define (rest-len rest) (ly:grob-property rest
rest-len-prop))
              (lambda (rest-a rest-b)
                (eqv? (rest-len rest-a) (rest-len rest-b)))))
          (mmrest-same-length (rest-eqv 'measure-count))
          (rest-same-length (rest-eqv 'duration-log)))

   (define (merge-mmrests rests)
...

https://codereview.appspot.com/321930043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Create engravers for merging rests (issue 321930043 by horndude77@gmail.com)

David Nalesnik
In reply to this post by pkx166h
Looks much better -- see comments below.


https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm
File scm/scheme-engravers.scm (right):

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm#newcode167
scm/scheme-engravers.scm:167: (lambda (rest) (ly:grob-set-property! rest
'Y-offset (rest-offset rest)))
Just moving one rest on top of the other might cause offsets with some
printers.  This happened at one time with flags on chords: see
http://lists.gnu.org/archive/html/bug-lilypond/2015-08/msg00080.html

Would it work to set 'stencil of all of the cdr to point-stencil?

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm#newcode193
scm/scheme-engravers.scm:193: ((eq? 'MultiMeasureRest (grob::name grob))
The only two grobs you could get are Rest and MultiMeasureRest, so you
could just write

(else
   (set! [...]


[I think looking at the grob name is fine.]

https://codereview.appspot.com/321930043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Create engravers for merging rests (issue 321930043 by horndude77@gmail.com)

Jay Anderson
In reply to this post by pkx166h

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm
File scm/scheme-engravers.scm (right):

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm#newcode151
scm/scheme-engravers.scm:151: (define (rest-eqv rest-len-prop)
On 2017/05/20 12:18:33, thomasmorley651 wrote:
> The current patch-set fails with:
> "Variable used before given a value: rest-eqv"
> because subsequent usage of 'define ...' is equivalent to let not let*

> Moving (define (rest-eqv rest-len-prop) before the engraver-definiton
starts
> will work or something at the lines of

> ...
>    (let* ((rest-eqv
>             (lambda (rest-len-prop)
>               (define (rest-len rest) (ly:grob-property rest
rest-len-prop))
>               (lambda (rest-a rest-b)
>                 (eqv? (rest-len rest-a) (rest-len rest-b)))))
>           (mmrest-same-length (rest-eqv 'measure-count))
>           (rest-same-length (rest-eqv 'duration-log)))

>    (define (merge-mmrests rests)
> ...


Interesting. I didn't run into that error. I'm using the latest lilydev
which is using guile 2. That could be the difference. In any case I've
removed the methods so this shouldn't happen.

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm#newcode167
scm/scheme-engravers.scm:167: (lambda (rest) (ly:grob-set-property! rest
'Y-offset (rest-offset rest)))
On 2017/05/20 14:33:15, david.nalesnik wrote:
> Just moving one rest on top of the other might cause offsets with some
printers.
>   This happened at one time with flags on chords: see
> http://lists.gnu.org/archive/html/bug-lilypond/2015-08/msg00080.html

> Would it work to set 'stencil of all of the cdr to point-stencil?

Using point-stencil causes odd alignment issues with text connected to
multimeasure rests. Making them invisible doesn't have this issue. Would
that solve the above potential problem?

Related to this, is there a way to re-parent grobs onto the surviving
rest? If so we could then use point-stencil (or possibly fully destroy)
the other rests.

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm#newcode193
scm/scheme-engravers.scm:193: ((eq? 'MultiMeasureRest (grob::name grob))
On 2017/05/20 14:33:15, david.nalesnik wrote:
> The only two grobs you could get are Rest and MultiMeasureRest, so you
could
> just write

> (else
>    (set! [...]


> [I think looking at the grob name is fine.]

It was simple enough to switch over and use (else ...) as well.

https://codereview.appspot.com/321930043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Create engravers for merging rests (issue 321930043 by horndude77@gmail.com)

Thomas Morley-2
In reply to this post by pkx166h
On 2017/05/21 04:27:33, horndude77 wrote:

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm
> File scm/scheme-engravers.scm (right):


https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm#newcode151
> scm/scheme-engravers.scm:151: (define (rest-eqv rest-len-prop)
> On 2017/05/20 12:18:33, thomasmorley651 wrote:
> > The current patch-set fails with:
> > "Variable used before given a value: rest-eqv"
> > because subsequent usage of 'define ...' is equivalent to let not
let*
> >
> > Moving (define (rest-eqv rest-len-prop) before the
engraver-definiton starts
> > will work or something at the lines of
> >
> > ...
> >   (let* ((rest-eqv
> >            (lambda (rest-len-prop)
> >              (define (rest-len rest) (ly:grob-property rest
rest-len-prop))
> >              (lambda (rest-a rest-b)
> >                (eqv? (rest-len rest-a) (rest-len rest-b)))))
> >          (mmrest-same-length (rest-eqv 'measure-count))
> >          (rest-same-length (rest-eqv 'duration-log)))
> >
> >   (define (merge-mmrests rests)
> > ...
> >

> Interesting. I didn't run into that error. I'm using the latest
lilydev which is
> using guile 2. That could be the difference. In any case I've removed
the
> methods so this shouldn't happen.

Yep, I can confirm it worked with guilev2, and your recent code works
for guilev1 as well.



https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm#newcode167
> scm/scheme-engravers.scm:167: (lambda (rest) (ly:grob-set-property!
rest
> 'Y-offset (rest-offset rest)))
> On 2017/05/20 14:33:15, david.nalesnik wrote:
> > Just moving one rest on top of the other might cause offsets with
some
> printers.
> >  This happened at one time with flags on chords: see
> > http://lists.gnu.org/archive/html/bug-lilypond/2015-08/msg00080.html
> >
> > Would it work to set 'stencil of all of the cdr to point-stencil?

> Using point-stencil causes odd alignment issues with text connected to
> multimeasure rests. Making them invisible doesn't have this issue.
Would that
> solve the above potential problem?

> Related to this, is there a way to re-parent grobs onto the surviving
rest? If
> so we could then use point-stencil (or possibly fully destroy) the
other rests.


We have ly:grob-set-parent!
My first attempts to use it were only partly successfull.
I'll attach my attempt at the issue tracker.
https://sourceforge.net/p/testlilyissues/issues/1228/?limit=25&page=1#d443
It's nothing more than a sketch and only tries to care about
MutiMeasureRestText, not MultiMeasureRestNumber.
Something weird happens at line-break.
Anyway, maybe of some use, though...




https://codereview.appspot.com/321930043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Create engravers for merging rests (issue 321930043 by horndude77@gmail.com)

Thomas Morley-2
In reply to this post by pkx166h
I'd like to mention another point:
What to do with pitched rests and rests with user-set staff-position,
merge them automatically to the zero-position?

I'd say using suspendRestMerging-property is sufficient to cover this
case, but this is only me. Other opinions?

https://codereview.appspot.com/321930043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Create engravers for merging rests (issue 321930043 by horndude77@gmail.com)

tdanielsmusic
In reply to this post by pkx166h
On 2017/05/21 17:12:26, thomasmorley651 wrote:
> I'd like to mention another point:
> What to do with pitched rests and rests with user-set staff-position,
merge them
> automatically to the zero-position?

If a user has explicitly set the position of a rest this should be
honoured
by default, I think ...

> I'd say using suspendRestMerging-property is sufficient to cover this
case, but
> this is only me. Other opinions?

... unless some property (mergePitchedRests?) is set true.


https://codereview.appspot.com/321930043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Create engravers for merging rests (issue 321930043 by horndude77@gmail.com)

David Nalesnik
In reply to this post by pkx166h

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm
File scm/scheme-engravers.scm (right):

https://codereview.appspot.com/321930043/diff/100001/scm/scheme-engravers.scm#newcode167
scm/scheme-engravers.scm:167: (lambda (rest) (ly:grob-set-property! rest
'Y-offset (rest-offset rest)))
On 2017/05/21 04:27:33, horndude77 wrote:
> On 2017/05/20 14:33:15, david.nalesnik wrote:
> > Just moving one rest on top of the other might cause offsets with
some
> printers.
> >  This happened at one time with flags on chords: see
> > http://lists.gnu.org/archive/html/bug-lilypond/2015-08/msg00080.html
> >
> > Would it work to set 'stencil of all of the cdr to point-stencil?

> Using point-stencil causes odd alignment issues with text connected to
> multimeasure rests. Making them invisible doesn't have this issue.
Would that
> solve the above potential problem?

I'm not so sure now that I identified an actual problem, but this an
improvement b/c it will cut down on PDF file size.

> Related to this, is there a way to re-parent grobs onto the surviving
rest? If
> so we could then use point-stencil (or possibly fully destroy) the
other rests.

I'd have to look into this, but as a first observation I think it would
help if MultiMeasureRest stored pointers to associated
MultiMeasureRestText and MultiMeasureRestNumber grobs.  (This would be
done in the Multi_measure_rest_engraver.)

I don't think that this is something that needs to be tackled in this
patch.

https://codereview.appspot.com/321930043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Create engravers for merging rests (issue 321930043 by horndude77@gmail.com)

Jay Anderson
In reply to this post by pkx166h
> On 2017/05/21 17:12:26, thomasmorley651 wrote:
> > I'd like to mention another point:
> > What to do with pitched rests and rests with user-set
staff-position, merge
> them
> > automatically to the zero-position?

> If a user has explicitly set the position of a rest this should be
honoured
> by default, I think ...

Done. If the staff-position property is set it won't attempt to merge
rests at that position. Seems to work well.

> > I'd say using suspendRestMerging-property is sufficient to cover
this case,
> but
> > this is only me. Other opinions?

> ... unless some property (mergePitchedRests?) is set true.

I didn't create another property for overriding this behavior. Merging
pitched rests could behave unexpectedly for some users - should one of
the merged rests pitches be respected or should they be at the single
voice position? Also, I'd hate to add a property like this which I'd
expect to be rarely used. It can be worked around with tags (i.e.
pitched rest for the single part, normal rest for combined part). If in
the future there's a need it could be added when the use cases are
better understood.

https://codereview.appspot.com/321930043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Create engravers for merging rests (issue 321930043 by horndude77@gmail.com)

David Kastrup
In reply to this post by pkx166h
On 2017/05/27 20:03:43, horndude77 wrote:
> > On 2017/05/21 17:12:26, thomasmorley651 wrote:
> > > I'd like to mention another point:
> > > What to do with pitched rests and rests with user-set
staff-position, merge
> > them
> > > automatically to the zero-position?
> >
> > If a user has explicitly set the position of a rest this should be
honoured
> > by default, I think ...

> Done. If the staff-position property is set it won't attempt to merge
rests at
> that position. Seems to work well.

> > > I'd say using suspendRestMerging-property is sufficient to cover
this case,
> > but
> > > this is only me. Other opinions?
> >
> > ... unless some property (mergePitchedRests?) is set true.

> I didn't create another property for overriding this behavior. Merging
pitched
> rests could behave unexpectedly for some users - should one of the
merged rests
> pitches be respected or should they be at the single voice position?
Also, I'd
> hate to add a property like this which I'd expect to be rarely used.
It can be
> worked around with tags (i.e. pitched rest for the single part, normal
rest for
> combined part). If in the future there's a need it could be added when
the use
> cases are better understood.

We have a similar situation regarding the directions of slurs.  In
this case, explicit different directions (in the SlurEvent expression,
not in the generated Slur grob) lead to both slurs being retained.

I think it reasonable to retain all unique pitched rests, and when
there are none, a single unpitched rest.  Is this useful?  I don't
really know.


https://codereview.appspot.com/321930043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel
Reply | Threaded
Open this post in threaded view
|

Re: Create engravers for merging rests (issue 321930043 by horndude77@gmail.com)

Thomas Morley-2
In reply to this post by pkx166h
I think this one warrants an entry in changes.


https://codereview.appspot.com/321930043/diff/140001/scm/scheme-engravers.scm
File scm/scheme-engravers.scm (right):

https://codereview.appspot.com/321930043/diff/140001/scm/scheme-engravers.scm#newcode152
scm/scheme-engravers.scm:152: (define (rests-all-unpitched rests)
This will catch not only pitched rests, but also rests where the user
has set Rest.staff-position.
I tend to agree with it, although rests where the user changed 'Y-offset
or 'extra-offset are not taken into account. We can't foresee any of
those possibilities, anyway.

Though, I would insert a comment about it for future developers working
on it.

In general, why do you use a recursion?
(every (lambda (r) (null? (ly:grob-property r 'staff-position))) rests)
looks at least shorter

https://codereview.appspot.com/321930043/

_______________________________________________
lilypond-devel mailing list
[hidden email]
https://lists.gnu.org/mailman/listinfo/lilypond-devel