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

Place cross staff dots correctly #19333

Merged
merged 4 commits into from
Oct 30, 2023

Conversation

miiizen
Copy link
Contributor

@miiizen miiizen commented Sep 8, 2023

Resolves: #19057
Dot layout now takes cross staff notes from staves above and below into account.

  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

@oktophonie oktophonie added the vtests This PR produces approved changes to vtest results label Sep 11, 2023
bool hasCrossNotes = c->staffMove();
Measure* m = c->measure();
if (!hasCrossNotes) {
for (size_t i = c->vStaffIdx() + 1 * VOICES; i < c->vStaffIdx() + 2 * VOICES; ++i) {
Copy link
Contributor

@cbjeukendrup cbjeukendrup Oct 12, 2023

Choose a reason for hiding this comment

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

It looks like some parentheses are missing here; adding 1 * VOICES to something that is a staff index seems not a logical thing to do.

Suggested change
for (size_t i = c->vStaffIdx() + 1 * VOICES; i < c->vStaffIdx() + 2 * VOICES; ++i) {
for (size_t i = (c->vStaffIdx() + 1) * VOICES; i < (c->vStaffIdx() + 2) * VOICES; ++i) {

But: why would you look at the staff below the "visual staff" of the item in question? Note that cross-staff notation can be created not only by moving notes to the staff below, but also by moving to the staff above. And both can happen simultaneously in the case of a three-staff instrument:
Scherm­afbeelding 2023-10-12 om 18 29 35
(Or perhaps I'm misunderstanding it completely; excuse me in that case)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The chord passed into getNoteListForDots is the top chord (lowest track number) on the stave, even if that top chord has been moved down from the stave above, so we can tell if that chord is cross staff easily. This loop is to check through all the voices on the staff below the one we are laying out to see if any voices have been moved up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Although, when doing some performance testing I realised this wasn't confined to the current part. It is now.

@miiizen miiizen force-pushed the 19057-cross-staff-dots branch 2 times, most recently from 178690d to e76fdda Compare October 23, 2023 15:59
@miiizen
Copy link
Contributor Author

miiizen commented Oct 23, 2023

Results of some performance testing.

Beethoven 9:
MASTER average: 17136.7288ms
PR average: 17107.996ms

A score of ~2500 bars of cross staff dotted notes:
Screenshot 2023-10-23 at 17 03 07

MASTER average: 765.211ms
PR average: 762.2924ms

Doesn't look like there's too much change compared to master, especially after the last improvement.

miiizen and others added 3 commits October 29, 2023 14:11
Move `m` up so that it can be reused; rename it to `measure`; inline `part`
@igorkorsukov igorkorsukov merged commit c5a87cd into musescore:master Oct 30, 2023
10 of 11 checks passed
@oktophonie oktophonie removed the request for review from RomanPudashkin October 30, 2023 13:57
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