Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add check for complex alternate endings #306

Merged
merged 3 commits into from
Jan 4, 2022

Conversation

nbrunett
Copy link
Contributor

@nbrunett nbrunett commented Sep 5, 2020

Description of Change(s)

The original logic in RepeatIndexer did not check for additional repeat ending bar lines once the number of alternate endings encountered was equal or greater than the number of repeats. I have added a check for that (similar to systemrenderer::drawAlternateEndings) as well as adding those ending bar lines to the repeat.

A bit of a tangent, but the attached file (alternate_endings2.zip) has the original score from the issue plus some other tests I thought up quickly. Everything except the 4th system works as I expect with this patch. I'm not sure if that one case is really related to this, or maybe it's just a poor choice of typesetting that doesn't need to be handled.

Fixes Issue(s)

@cameronwhite
Copy link
Member

Thanks for looking at this!
I think the logic here is good, just a couple questions / thoughts:

  • Since nested repeats are allowed, could this change incorrectly cause the outer repeat end bar to be associated with the inner section? Perhaps the check should be for the existence of more alternate endings, rather than repeat end bars?
  • Should the search iterate into the following systems instead of just checking the later bars in the current system?

I think the case that doesn't work in alternate_ending2.zip is a bug, but probably is an issue that can be fixed separately (maybe in RepeatedSection::performRepeat()). I suspect there's an assumption that the final alternate ending is at the end of the section, so it isn't jumping past the other alternate endings.

@nbrunett
Copy link
Contributor Author

nbrunett commented Sep 7, 2020

Good points. The latest change should actually loop through the current and following systems until the end of the score or until it encounters a repeat start or end. So this will handle alternate endings that span multiple systems.

I'm sorry but I don't follow what you are suggesting for dealing with nested repeats. Thinking about this a bit has me wondering if it would be better to have alternate endings include an end position/barline attribute so we don't have to try inferring where it would be.

@cameronwhite
Copy link
Member

For nested repeats I was meaning something like these files (nested_repeats.zip), although it seems like there are some existing bugs there - it's not a very common thing to use.

The change looks okay - it might be cleaner as a separate function so that the flag variables like doneScanning can be eliminated.
It might be good to also cache the state of whether another repeat end is expected - otherwise this could turn into a quadratic algorithm where on each iteration it searches through the reset of the barlines in the score.

@nbrunett
Copy link
Contributor Author

Thanks for giving some examples. I was actually asking about how searching for later alternate endings could be used to determine if the current alternate ending group was done.

Comparing "alternate_endings_nested" between alpha 13 and these changes here, it plays closer to how it should with the current changes. The alternate endings play properly but the outer repeat is ignored. "alternate_endings_nested2" works the same with alpha 13 and with these changes. There are still bugs, but the original issue should be addressed.

I've moved the check for following repeat ends into a function, but let me know if it is too specific.

On the point about caching the expectation for repeat endings, shouldn't the search through following barlines only happen when the alternate ending count >= the number of repeats? So it should just be done when we are trying to find the end of the current alternate ending group, I believe.

@cameronwhite
Copy link
Member

Thanks, I think the changes look good!

For the nested repeats, my thinking was that the "alternate ending count >= # of repeats" check could be problematic if the inner repeated section had alternate endings. The repeat end bar from the outer section could prevent the inner repeated section from being popped off the stack, since it thinks there are more repeat end bars coming for the inner section?

@cameronwhite
Copy link
Member

Revisiting this, addressing the nested repeat issue I mentioned requires just a small change to the logic. We only should search for the next repeat end bar if the last alternate ending requires it (e.g. 2,4 requires a repeat end bar, but just 4 doesn't require it if 4 is the highest repeat number
I'll merge this and make the additional adjustments - thanks for your work on looking into this!

@cameronwhite cameronwhite merged commit 3dbb73f into powertab:master Jan 4, 2022
@nbrunett
Copy link
Contributor Author

nbrunett commented Jan 4, 2022

Glad it helped, sorry it fell to the wayside before being wrapped up.

@nbrunett nbrunett deleted the fix-repeats branch January 4, 2022 13:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants