8604 improve volta unwinding #3371
Conversation
* no-repeat (13) shouldn't have an off-page invalid jump either * Jumps should be taken on last repeat * repeatlist should make human-sense, not (only) programmer-sense (9) * added 1,3/2,4 style test * added tests to determine when an open volta should end * added 'with repeats' tests
|
As it never worked as intended prior to this PR, I tend to think that we should fix it for 2.2 too, even if that means that playback may render differently, due to the tricks involved getting it to work half way decent in pre 2.2. Together with a sentence about 'Known incompatibilities' in the Release Notes of course... |
| @@ -30,7 +30,7 @@ Volta* Score::searchVolta(int tick) const | |||
| { | |||
| for (const std::pair<int,Spanner*>& p : _spanner.map()) { | |||
| Spanner* s = p.second; | |||
| if (s->type() != ElementType::VOLTA) | |||
| if (!s->isVolta()) | |||
There was a problem hiding this comment.
I believe this and some more isXxx() and toXxx() won't fit for 2.2.
There was a problem hiding this comment.
indeed.. if desired by @lasconic I could rework this into two separate commits, then the first one would remain 2.x compatible and all "codestyle" changes into a 2nd commit?
There was a problem hiding this comment.
I can handle this. Give me a couple of days to try the PR and see how much it improve the current situation and break compatibility.
|
FWIW, I definitely applaud any improvement that can be made here, and would be willing to bear the pain of some incompatibility if it means we can finally have a more intuitive and logical story to tell. Right now, of all the FAQ's we get, it is the questions about volta behavior that bother me the most, because we just don't have great answers. However, completely breaking compatibility - so that even the most basic tunes with a single repeat and two voltas no longer works - would make me uncomfortable. I'd like to understand better just how bad it will be. |
|
It will 'break' the most simple volta song from 2.x if the volta properties for it had been altered to achieve correct playback in 2.x If you simply had consecutive voltas, 2.x required you to set the repeat lists as follows: The bonus is that this currently is how the repeat lists are set for the palette items in 2.x already. So a user would in the most basic scenarios no longer be required to fiddle with the repeatlist property at all. |
…6, fix #148276, fix #230531, fix #267778: improve volta playback unwindSection using RepeatList as how a human will notate the volta: using the n-th time a startRepeat is taken, also slightly modified the jump detection logic to have it check for jumps only upon the last passage of a measure
jeetee
force-pushed
the
8604_improve_volta_unwinding
branch
from
5b60e48
to
53d34d4
Compare
|
So this PR will break the playback of 2.x scores that used a workaround to get the playback right prior to this fix. IMHO this is a downside we can quite easily accept, we don't really need to maintain bug compatibility ;-) |
| @@ -112,14 +112,6 @@ | |||
| </BarLine> | |||
| </Measure> | |||
| <Measure number="5"> | |||
| <Jump> | |||
There was a problem hiding this comment.
I don't get that. Why is it removed?
There was a problem hiding this comment.
Ok now I see the commit message "no-repeat (13) shouldn't have an off-page invalid jump either". In fact, this test was there to make sure that meaningless jump would do nothing. It should stay like this, and indeed the jump shouldn't be offpage.
|
Except my comment on test repeat13.mscx, it works great. I'm checking the code, and what needs to be done for 2.2. |
| if (m->prevMeasure()->sectionBreak()) | ||
| break; | ||
|
|
||
| while ( (!m->repeatStart()) |
| void repeat44() { repeat("repeat44.mscx", "1;2;3;4;5;3;6;3;4;1;7"); } // Jump from within a volta | ||
|
|
||
| void repeat45() { repeat("repeat45.mscx", "1;2;3;4;3;5;6;2;3;4;3;5;6;7"); } // repeat12 but with 'play repeats' enabled | ||
| void repeat46() { repeat("repeat46.mscx", "1;2;3;4;5;3;4;2;3;4;5;3;4;5;6"); } // repeat24 but with 'play repeats' enabled |
|
Here is the PR #3442 |
Fix several issues with repeat and volta. Counterpart of PR #3371
no file format changes required, so should quite cleanly apply to 2.x as well; but does change volta interpretation.
Where 2.x currently requires the repeatlist of a volta to be set to the n-th time that measure/volta is encountered, this PR requires the repeatlist to reflect the human-readable logic of being the x-th repeat in total.
Applying this to 2.x will result in scores pre 2.2 with volta rendering a different playback roadmap than scores from 2.2+