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

alphaTex Alternate Endings #791

Merged
merged 7 commits into from
May 7, 2022
Merged

Conversation

jonaro00
Copy link
Contributor

Issues

Fixes #671

Proposed changes

The \ae bar meta tag for alphaTex can be used to mark bars as alternate endings in a repeat.
Syntax examples: \ae (1 2 3) and \ae 4.

Checklist

  • I consent that this change becomes part of alphaTab under it's current or any future open source license
  • Changes are implemented
  • Existing builds tests pass
  • New tests were added
    • Two tests for parsing the tag.
    • A new test for MidiPlaybackController added but needs a fix for passing.

Further details

  • This is a breaking change
  • This change will require update of the documentation/website
    • I can write a section for this once the feature has been released.

@Danielku15
Copy link
Member

I had now a look at your tests and it seems the alternate endings are not yet correctly applied. From a visual appearance of the file, I created a matching file in Guitar Pro. The correct order which GP and alphaTab should have, is:

let expectedBars: number[] = [
    0, 4, 8, // First round: 1st, 5th and 9th bar which have the ending for 1. 
    1, 5, 9, // Second round: 2nd, 6th and 10th bar which have the ending for 2
    2, 3, 6, 7, 8 // Third round: 3rd, 4th, 7th, 8th and 9th which have the ending for 3. 
                    // 3rd and 8th bar don't have the ending explicitly
                    // but extended from the previous bar. 
];

Following rule seems the key difference:

  • If an alternate ending is applied to a bar, it is applied also to all subsequent bars until either the repeat close appears or a different alternate ending is specified.

In your test you expect the bars to be played which do not have any alternate endings defined, but that's not correct. The alternate endings stay valid.

I attached you the GP7 file where I followed the actions described also in your alphaTex file. For the bars where you have defined the \ae, I also opened the alternate endings editor and checked the bars. You can check how GP might have applied these endings then to the bars.

AlternateEndings.zip

@jonaro00
Copy link
Contributor Author

Alright, I see I've misunderstood the key difference. I was thinking that all bars in the AE would need to be explicitly included with an \ae tag. If that was the case, then the \ae tag could technically have been used to construct an "alternate beginning" (and more), but now I see they are forced to continue until the next \ae or \rc tag.

So, when an \ae tag is encountered by the parser, should the bitflag be set on all subsequent bars until either the repeat close appears or a different alternate ending is specified or should they be set only on the initial bar, as it is currently?
When pasting my test case in the playground, the bars are played in the correct order that you wrote (I will put that correct order in the test case). The only thing is that the visual lines above the staff could be extended over the 4th and 8th bars.

Also, in the test case run, playedBars ended up with the values [0,1,2,3,4,5,6,7,8,9 ... (repeated 3 times), which seems strange to me.

I don't have GuitarPro, so I can't verify stuff in there, but I can load the gp file in alphaTab and check the resulting Model.

@Danielku15
Copy link
Member

As said: I would recommend comparing the alphaTex model to the one of GP and try to follow the same principle. 😉 The file was created by doing the same actions as specified also in alphaTex. Guitar Pro might also apply then the AEs explicitly to the subsequent bars as per the given rule. In best case the models are with the equal flags.

@jonaro00
Copy link
Contributor Author

jonaro00 commented Mar 14, 2022

Thanks for the guidance. I have now compared the models and understood the correct way to set flags. I also learned that the visuals of the repeats are also correct, so no issues remain.

I refactored some code in the test case and fixed the previously incorrect counting of played bars. Now, the testRepeat and testAlphaTexRepeat functions are very similar, and they could be rewritten and combined into one function. Do you think that is suitable or do we leave it like this?

@Danielku15
Copy link
Member

Do you think that is suitable or do we leave it like this?

Avoiding DRY is always good. I like the principle of also writing test code clean like the production code. With tests it is often a bit difficult as you still want to keep them isolated/independent but keep the maintenance effort low.

I think in this case it would make sense to try use testRepeat within testAlphaTexRepeat and maybe take over some aspects into testRepeat which were missing there until now (e.g. the early exit path when we detect too many bars being already generated).

@jonaro00 jonaro00 marked this pull request as ready for review March 17, 2022 20:41
Copy link
Member

@Danielku15 Danielku15 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good now. @jonaro00 Is there anything you still want to extend/improve on this PR or is is also fully ready for integration from your side?

@jonaro00
Copy link
Contributor Author

jonaro00 commented May 7, 2022

All looks good. You can go ahead and merge. 😄

@Danielku15 Danielku15 merged commit fc824dc into CoderLine:develop May 7, 2022
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