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

Breve-rest corrections #25374

Merged
merged 2 commits into from
Nov 5, 2024
Merged

Breve-rest corrections #25374

merged 2 commits into from
Nov 5, 2024

Conversation

mike-spa
Copy link
Contributor

Resolves: #25246
Resolves: #25294

This is a stripped down version of #25295.

@mike-spa mike-spa marked this pull request as ready for review October 29, 2024 18:03
@rpatters1
Copy link
Contributor

rpatters1 commented Oct 29, 2024

I think you should include more of the visual tests I added in #25295. In particular, there are currently no visual tests for when Sid::multiVoiceRestTwoSpaceOffset is true. Without such tests, we could be changing the rest behavior without any idea that we are doing so. I created b versions of some of the rest tests for that are identical to their counterparts except the 2-space offset style is true. I don't care what naming convention we use, but here are the ones I added:

  • rest-02b.mscz
  • rest-09b.mscz

In addition, I added rest-10.mscz and rest-10b.mscz to test floating full measure rests inside the staff for both styles. Of all the tests I added, perhaps these could be omitted. But there are no other tests for fullmeasure rests among the rest- tests.

Ultimately, because there have until now been no regression tests for when Sid::multiVoiceRestTwoSpaceOffset is true, I am uncertain if the current behavior is by design and/or even desirable. To me it seems like the rests float way too far outside the staff. But whatever the answer is, I think tests should be added so that if the behavior changes, we'll know it.

@oktophonie oktophonie added the vtests This PR produces approved changes to vtest results label Oct 30, 2024
@@ -486,17 +486,18 @@ int Rest::computeVoiceOffset(int lines, LayoutData* ldata) const

int Rest::computeWholeRestOffset(int voiceOffset, int lines) const
Copy link
Contributor

@rpatters1 rpatters1 Oct 30, 2024

Choose a reason for hiding this comment

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

In my version of this PR, I changed the name from computeWholeRestOffset to computeWholeOrBreveRestOffset, to reflect what its new function is.

@miiizen miiizen merged commit a8e1c85 into musescore:master Nov 5, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
vtests This PR produces approved changes to vtest results
Projects
None yet
4 participants