[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: Issue 4205: Improve part combiner's rest analysis (issue 174610043 b

From: Urs Liska
Subject: Re: Issue 4205: Improve part combiner's rest analysis (issue 174610043 by address@hidden)
Date: Mon, 24 Nov 2014 10:11:29 +0100
User-agent: Mozilla/5.0 (X11; Linux i686; rv:24.0) Gecko/20100101 Thunderbird/24.6.0

I don't know how to properly review this, but I checked against a current score I'm working on, and I see it removes one of the most annoying issues I had so far.

Consider this snippet:

\version "2.19.16"
one = \relative c'' {
  c2 c
  r8 c b a g f e d

two = \relative c' {
  c2 c

\score {
  \new Staff \partcombine \one \two

This previously (incl. current master) resulted in the attached partcombine-fail-merge-rests.png, which is musical nonsense.
With the patch applied it results in partcombine-merge-rests.png.
So I think the patch does what it says (and helps me very much), so LGTM.

However, there's an issue with this example. I'm sure it's not related to the patch but an independent issue - but maybe you could try to fix that along with your patch?

When the partcombiner deals with a section that starts with different rests (the situation you improve in your patch) it fails to set the Voice number correctly. I even have the impression it actively sets it wrongly. The attached image shows that - at the first moment of the measure the voices are set correctly: The quaver rest is \voiceOne, the full measure rest is \voiceTwo.
But the notes starting with the second quaver are obviously \oneVoice.

It is clear that the partcombiner actively sets the upper voice to \oneVoice because

\voiceOne r8 c b a g f e d

doesn't change the output while

r8 \voiceOne c b a g f e d


c8 c b a g f e d

both produce the correct result.


Do you think you could tackle that in the context of your current patch?
Otherwise could please someone open a new issue for that (CCing to bug-lilypond)


Am 24.11.2014 06:12, schrieb address@hidden:
Reviewers: ,

Issue 4205: Improve part combiner's rest analysis

Rests must begin and end simultaneously to be merged into the shared

Rests, skips, and multi-measure rests are kept apart even if they
begin and end simultaneously.

This does not produce ideal output in every case, but it avoids
producing musical nonsense.

Please review this at https://codereview.appspot.com/174610043/

Affected files (+131, -4 lines):
  A input/regression/part-combine-mmrest-shared.ly
  A input/regression/part-combine-silence.ly
  A input/regression/part-combine-silence-mixed.ly
  M scm/part-combiner.scm

lilypond-devel mailing list

Attachment: partcombine-fail-merge-rests.png
Description: PNG image

Attachment: partcombine-merge-rests.png
Description: PNG image

reply via email to

[Prev in Thread] Current Thread [Next in Thread]