Section break causes pause before repeats

• Sep 9, 2014 - 08:38
Priority
P1 - High
Type
Functional
Frequency
Many
Severity
S3 - Major
Status
active
Regression
No
Workaround
No
Project

4893521930

  1. Create a score with a repeat barline at the end
  2. Add a section break and another section.
  3. Add some notes
  4. Save
  5. Reopen
  6. Play

Expected result: the repeat is played, a pause is marked and the second section is played
Actual result: the first time is played, a pause is marked, the second time is played, a pause again, and the second section is played.

Example score : http://musescore.org/sites/musescore.org/files/Musiques%20du%20Dauphin%…


Comments

I've noticed this issue as well. It happens not just on repeat signs but jumps as well (e.g. attached test with D.C. and D.S.). I've looked through the code, and discovered that it occurs because the section break pauses are implemented as setPause on the final tick of the measure ending with the section break. Therefore any time that measure is encountered, the pause is incurred.

I don't think that final measure is the entity that needs to be paused. Rather the pause should logically occur after that measure (but before the first measure of the next section).

So my proposed solution is to insert an additional tick in the tempomap inbetween the final tick of the current section and the initial tick of the next section. (I would not be increasing the measure's internal length or otherwise interfere with the measure itself). This extra tick would the one that gets applied with setPause.

I will take care to remove this extra tick whenever the section break is removed (which also occurs when that measure is delted) as well.

Let me know if there is something wrong with this approach.

I don't understand what you mean "insert an extra tick". The final tick of the last measure *is* the first tick of the next. Eg, in 4/4 time, with 480 ticks per quarter note, the last segment of the first measure is at tick 1960, the next measure is *also* at tick 1960. Are you proposing shifting everything so that the next measure is actually at 1961? Actually changing the tick of that and every subsequent measure? I kind of doubt that would work well; there are probably any number of places in the code where it is assumed these values would be equal. Or do you mean something else?

Actually the real problem here is that the tempomap is indexed in ticks and not in uticks currently. If you don't know yet, tick are used for the graphical score, while utick are used for playback when the score is "unrolled". So at one give tick we currently have a pause in the tempo map. There is no way to know if we are going at utick 1920 or 3840 if these two uticks are at the same measure at tick 1920 for exemple...

If tempomap was using utick we could implement "slower the second time" and so on... but the code would be a lot messier...

>> "Are you proposing shifting everything so that the next measure is actually at 1961? Actually changing the tick of that and every subsequent measure?"

Yes, that is what I was saying.

>> "If tempomap was using utick we could implement "slower the second time" and so on... but the code would be a lot messier..."

Ok. Is this something that is on the roadmap that I can try making changes to implement? Or is this something that is wished for but will never be implemented for fear of breaking things? Just asking so I know best way to proceed. It indeed would be nice to have things like tempo changes be on a per repeat basis. (Would indexing by utick's also enable handling dynamic differences be triggered on per repeat basis, e.g. "f-p" for "play forte first time, piano second time?)

Since I don't want to interfere with any hidden assumptions marc brought up about "ticks", I'm going to try modifying the code just so the final "utick" is the entity that gets paused...

I can't speak for roadmap. It's not something I kno w of specific plans for, but the idea of doing this comes up from time to time, and maybe this would be an excuse to provide the framework. Now is as good a timre as any to give it a shot.

As for the tick shift plan, I don't *know* that it won't work, I was just expressing some doubts. The other tricky aspect about it is making sure to update the tick info for all spanners and all the other maps.

I have added a unrolledTempoMap to the root score, which maps unrolled TempoEvents indexed by uticks. Unrolled playback is working as expected, both with the two scores posted here as well as some faily complex tests I've made.

https://github.com/ericfont/MuseScore/tree/32696-unroll-TempoMap-after-…

An overview of changes:

  1. Section Breaks use a different type of pause from Caesuras, because they need to be handled differently when unrolling:
    • Caesuras use TempoType::PAUSE_BEFORE_TICK, which refers to pauses that should be incurred just before the tick is encountered. If playback encounters one before a repeat sign or jump, these pauses will always be incurred whether or not the repeat/jump is taken or not.
    • SectionBreaks use TempoType::PAUSE_THROUGH_TICK, which refer to pauses that should only be incurred when playback crosses through the tick. If playback encounters one at a repeat sign or jump, these pauses will not be incurred when the repeat/jump is taken since playback doesn't actually cross through the tick that has the PAUSE_THROUGH_TICK event. But these pauses will occur when the repeat/jump is not taken, since playback will cross *through* the tick.
  2. The TempoMap class has a new constructor which regenerates an unrolledTempoMap using a repeatlist and graphical tempo map as inputs. This is called after repeat lists are unwound or when the someone calls the score's utick2utime or utime2utick methods, at which point the unrolledTempoMap is regenerated, and gets invalidated whenever the user adds another tempo event or measure, (and in a few other cases I need to look into further). Most of the functionality in the TempoMap class is shared by both the graphical tempomap and the unrolled tempomap. When unrolling, special care is taken to ensure that the final tempo event of the previously unrolled RepeatSection is correctly merged with the first tempo event of the next RepeatSection based on these rules:
    1. Always incur pause from breaths/caesuras prior to jumping:
    2. Never incur a pause from section breaks, as the jump occurs prior to encountering the break
    3. Ignore tempo FIX at end of previous RepeatSection. Only use tempo FIX from start of next RepeatSection.

    Copying the rest of the tempo events that occur within each RepeatSection is straightforward. Note: if want to trigger tempo events on specific repeats (e.g. "slower the second time"), then that can be handled here by selectively copyig the tempo events that are indicated to trigger on specific repeats. Currently my unrolling will observe tempo changes in order of the time that playback encounter their tempo events, so my code plays a repeat "slower the second time" if playback enounters a slower tempo marking and doesn't enounter another explicit tempo event for the original tempo.
    I'm not to worry about performance loss due to regenerating the unrolled tempo map, as it only gets regenerated after the user both makes changes to tempo/form and presses play (or does something else that calls utick2utime or utime2utick). I also don't image the unrolled tempomap to be larger than several hundred events, even on a large score.

  3. RepeatLists is no longer responsible for mapping time <-> utime, as that is now unrolledTempoMap's responsibility. I've removed those time mapping functions from RepeatLists and removed those time variables from the RepeatSection struct. RepeatList only needs to worry about unwinding repeats and for mapping ticks <->uticks.

I can provide more details if you wan't and can't figure out from reading my code. Note that fermatas will fit nicely in my framework as a "PAUSE_AFTER_TICK", since they would add a delay immediately after the tick of a Chord-Rest containing a fermata.

Notes: I'm not ready for a pull request yet, as I still have some few bugs/crashs, I think related to when I need to invalidate the unrolledTempomap and when I can regenerate it. mp3 export seems to work correclty. But midi file export doesn't hande the breaks, so I need to look into that (my suspicion is that midi export is using a different tempomap class in /miditools/tempomap.cpp, so I might look into merging the two together for consistency). Please take a look and give me any feedback. I'm tyring to follow the code formatting guide, and to write in a similar style as the other code I see, but please let me know if I'm doing any programming styles that are frowned upon and I can fix them.

@ericfonainejazz: thanks for spending time on this! It is appreciated.

For what is worth, from a quick glance at your code, no bell ringed, so please go on!

Existing code base tends to be rather terse in commenting and this makes some spots hard to understand; so, if anything, with respect to existing code, feel free to exceed with comments in your code!

To have a clearer global picture of the changes involved, it could be useful to squash the multiple commits into a single one ( http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.h… ); maybe not right now, but as soon as you will feel confident that the individual steps go in the right direction (the practice is to ask for squashed commits for the PR anyway).

Sorry I'm new to git. I'm using Eclipse egit plugin. I have just tried merging my commits into one using these directions I found onllilne:

"In History tab select all commits you want to squash. Then right-click the selected commits and from the context menu select Modify --> Squash."

So now my history view in git shows them all as one commit. I then pushed that commit to my github. But I don't know if that merged commit is succesfully merged as desired on github...could you check if that latest commit is correctly "merged'?

I've fixed the bugs, I believe. I've sqashed the commits into a new clean branch named "32696-unrolledTempomap" and issued a pull request https://github.com/musescore/MuseScore/pull/2133

Note: There are plently of qDebug messages I've left in my code...those can all be removed, but I'm leaving them in incase any bugs pop up. I think I'm invalidating the unrolledTempomap everytime a new tempoevent is inserted, and only regenerating it when needed (i.e. the play button gets pressed), but I haven't studied the rest of the codebase, so I may be ignorant of something important.

I didn't include any test files. I'm not sure how to create test files in the context of this issue for the auto-tester. Would I check the ending unrolled time of the final tick and compare that to an expected value? Should I make a bunch of smaller tests that only deal with each specific case seperately, or should I try to combine a bunch together into one file?

Note: I do notice some delay almost a second in a huge scores like opengoldberg after adding new tempo events and pressing play for the first time, but I actually think most of that delay is not due my code (I might try running a profiler and see if the delay in my function is significant...like >100ms or so).

(regarding midi file output, I notice that the 2.0.1 release doesn't include any section break delays in the midi file output anyway, so that issue specifically is not a problem with my code. I believe the difference with midi file output occurs because there are two tempomap classes. I might want to file another bug report that says midi file output should have identical pauses as in desktop playback, and fix it by merging the two tempomap classes.)

In light of the revelation that section breaks can occur on non-measure frames, I'm canceling my earlier pull request (https://github.com/musescore/MuseScore/pull/2133). Also another thing I now need to be aware of is: if a section ends on a fine, then it doesn't have a section break immediately after it, and hence would incorrectly not incur a pause when should actually have one. I will open up a new PR, and will probably implement my initially suggested "tick shift" method. Once my fix for #65161: Da Capo/Da Segno al Fine ignore next section on playback (https://github.com/musescore/MuseScore/pull/2172) gets merged, the modified unwind() will only produce RepeatSegments that never cross SECTION_BREAK boundaries. Therefore, I think I just would need to make a note during unwinding about which RepeatSegments start on a section break (could be a boolean variable in RepeatSegment, such as "isAfterSectionBreak"). Those RepeatSegments and their measures will have their initial tick value be increased by one, which could probably be handled when RepeatList::update() is called. This new fix will also fix #73806: SECTION_BREAK on non-measure MEASURE_BASE does not incur pause before starting next section

I will not try to generate an unrolled tempomap for my next PR...I'll leave that for another feature request.

I'm following the isAfterSectionBreak but the addition of a graphical tick, I don't get it. If it means that in the scheme measure(4/4), section break, measure, the second measure will have a tick of 1921, I don't think it's a good solution...

OK, then forget about that tick shift idea.

I am now considering a simpler solution. I will add isAfterSectionBreak boolean to RepeatSection. And then in RepeatList::update() if isAfterSectionBreak, then add delay to s->utime. There maybe something else I need to take care of as well, but how does that sound to you?

Status (old) active patch (code needs review)

I'm marking as "patch code needs review" because I am almost certain that whatever transient error that happened on my first commit to my latest PR (https://github.com/musescore/MuseScore/pull/2188) was not in a result of my code in the PR and is an entirely separate issue.

Regarding my comment in #15, my PR is slightly different since I forwent my suggestion of a boolean "isAfterSectionBreak" in RepeatSegment, and instead only have a qreal "pauseBefore" which equals 0 if there is no pause before the section.

Status (old) patch (code needs review) active

I should probably reset this status to "active" and unassign myself since my previous attempts to fix this weren't accepted. I'd like to attempt again with a different strategy if no one else takes it.

I would recommend to not bother to fix for 2.1, and instead save for 3.0, because I discovered that implementing a proper fix for this would require too many changes to underlying structures.

In reply to by Jojo-Schmitz

I think section breaks should be only a sort of "marker", telling simply MS to reset all parameters as if the next bar was the beginning of a score.
So during the playback, the logic would be :
- I'm playing...
- Woaw ! A section break ! Let's have a flag on!
- I'm still playing exactly as if nothing happend...
- Ok, I'm at the end of the playback. Do I have a SB flag on ? Yes : let's go to the 1st bar after the sction break marker.
- Let's reset all parameters
- Let's begin playing with brand new parameters.

If I'm not wrong, this should correct the behaviour, no?

After having spent some time with the unwind logic, I think the best tracking of when to honor the pause would indeed be on the RepeatSegment.
RepeatList::unwind() already loops over the score on a per-section base, so it is aware when we are moving from one section to the next.

Maybe not exactly the right topic here, but wouldn't it be an improvement resp. wouldn't it be more obviously for the user to see/adjust the section breaks properties inside the inspector (especially the settings for "pause")?

Just a thought from user's point of view (aware, that's maybe stupid):

Without being familiar with ticks (only with my personal "Ticks" (https://dict.leo.org/englisch-deutsch/Tick?side=right) ;-), an maybe it's already in a similar way discussed here, but why couldn't it be handled (and implemented in the code) with the same rules like a repeat sign and a jump in the same measure (for example a "repeat sign" and a "d.s.")?:

timelinesegno.png

That means: first play the repeats then jump to segno (anf for the section break: first play the repeats then pause)

In reply to by kuwitt

kuwitt: your idea is not wrong. To me it is quite clear what needs to happen in code, I just need to get around doing it.

  1. RepeatSegment add a property endsWithPause/Break, defaults to false.
  2. set to true by unwindlogic upon moving from the last measure of a section to the next (caveat, only set to true for 'finished' jumps, not during a jump)
  3. In rendering, lookup where the current pause is processed, alter the check by first using the repeatSegment.endsWithPause to know whether the pause has to be applied or not.

Implementation details on (3) are still a bit unresearched on my end:
* does a break influence uticks/tempomap? If not, we should be done fairly straight forward. If it does, we "just" need more time :-p

My feeling is that the sensible way to solve this is to separate the section break:
a) either from the measure, as a separate kind of frame which takes time rather than space
b) or from the measure properties into a segment of its own and coming after everything else, so all repeats, da capo etc are executed before the section break is reached (and "played") and execution would resume at it even after an "al Fine".