Skip to content

A complete overhaul of rest positioning logic #16719

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Mar 22, 2023
Merged

A complete overhaul of rest positioning logic #16719

merged 6 commits into from
Mar 22, 2023

Conversation

mike-spa
Copy link
Contributor

@mike-spa mike-spa commented Mar 9, 2023 •
edited
Loading

Resolves: #15301
Resolves: #14650

This PR contains a complete overhaul of the rest positioning logic, so it probably doesn't make much sense to explain in detail everething that's being done. But just as an overview:

  • The default vertical position of rest has been changed to be more consistent in several cases, especially with numbers of staff lines different than 5.
  • The code managing the default position has been scrapped and rewritten because it was inadequate, and several functions were performing competing (and conflicting) logic. Now a single function determines the default position, a single function determines (if needed) the voice offset, and a single function determines the whole-rest offset (if needed).
  • In multi-voice cases, rests used to be offset by 2sp up or down. Now they are offset by 1sp as default. This is configurable via a tiny new style option added to the Style -> Rest menu.
  • A separate function is dedicated to resolving rest VS note vertical collisions.
  • A separate function is dedicated to resolving rest VS rest vertical collisions.
  • When rests are under beams, beams used to be moved away (in MuseScore 3). That is wrong, so it was temporarily removed from MuseScore 4.0. Now, rests are moved away to avoid colliding with beams.
  • Only if rests can't be moved, beams are moved away.
  • A dedicated piece of logic controls the case of two rests in two voices both beamed, as it requires a whole procedure of its own.
  • Now situations with beamlets are much better managed both in terms of vertical and horizontal spacing.

A whole bunch of test failures is expected. I'm also going to add some more visual tests to cover all the new things I've done.

This PR includes all the work done in #16410 so I'm going to close that one.

(Still to come: logic for alignment of adjacent rests)

UPDATE: the last commit features an additional setting exposed in the Style -> Rest menu (as by @bkunda 's spec), made from a combination of QML and Widgets. It is literally copy-pasted from the implementation of the straight VS traditional flags in Style -> Note.

20230316_152301.mp4

@mike-spa mike-spa requested a review from its-not-nice March 9, 2023 13:02
@mike-spa mike-spa mentioned this pull request Mar 9, 2023
Closed
@mike-spa mike-spa force-pushed the verticalRestPositioning branch 7 times, most recently from d1befce to ee33a8c Compare March 15, 2023 15:44
@its-not-nice its-not-nice added the vtests This PR produces approved changes to vtest results label Mar 15, 2023
@mike-spa mike-spa force-pushed the verticalRestPositioning branch 3 times, most recently from aef89ab to 127996b Compare March 20, 2023 09:00
@mike-spa
Copy link
Contributor Author

Profiler logs on Beethoven 9 score:

MASTER:
master

THIS PR:
image

@abariska
Copy link

@mike-spa Seems like performance is affected.
I also tested on Bethoven 9 symphonie. So, if talk about macOS , opening time is almost the same with master (36.8 agains 35.8), but layout recalculation time is noticeably bigger (e.g. 61sec against 50sec). I changed leading space for entire selected score from 0 to 5. Similar situation with Windows and Linux.
Could you try to reduce layout recalculation time?
abariska 2023-03-20 at 18 38 29

@mike-spa mike-spa force-pushed the verticalRestPositioning branch 2 times, most recently from a188a21 to 716ea0a Compare March 21, 2023 14:28
@mike-spa
Copy link
Contributor Author

I think I've understood what was happening with the relayout recalculation time.

  1. Essentially, I need the BeamSegment class to have a shape() method, so I made it a child of EngravingItem, which lets me inherit shape(). But EngravingItem is a big class with a lot of other stuff that BeamSegment doesn't need (and there are a lot of BeamSegments in a score). Also, we really never use BeamSegment::shape() without knowing that it is a BeamSegment, so there is no point in having it virtual. End of the story: I just removed the EngravingItem inheritance, made shape() a non-virtual method, and this recovered basically all the lost performance.
  2. There are a lot of BeamSegments in a score, and they are cancelled and recreated at every layout, so it's worth using the custom memory allocator (tiny improvement, but still an improvement).

I've added a commit which fixes this (for @RomanPudashkin to check).
@abariska let me know if things also look better for you now.

@RomanPudashkin RomanPudashkin requested a review from abariska March 21, 2023 14:55
mike-spa added 5 commits March 22, 2023 09:12
@mike-spa
A complete overhaul of rest vertical positioning
40b3ee6 
Improved collision management between beam, beamlets and rests

Fix beam VS horizontal spacing glitches

Correct MMRest vertical placement

The Rest::getSymbol method should just compute the symbol.
The function should do only one thing, i.e. return the symbol. It shouldn't do vertical position calculations.

Introducing Rest::computeNaturalLine
This deals exclusively with the "natural" position (i.e. when the measure features only a single voice)

Rest::computeLineOffset refactored to Rest::computeVoiceOffset
This function was completely incoherent: it was computing voice offsets, but also vertical collision avoidance, and was also interfering with the natural rest position. Now the function does only one thing, i.e. compute the correct voice offset.

Move whole rest offset logic to dedicated Rest::computeWholeRestOffset

Create style setting for multi-voice rest offset

Resolve rest-to-note vertical conflicts

Resolve rest-to-rest vertical conflicts

Resolve rest-to-beam conflicts in multi-voice situations

If rests can't be moved, move beams away

Resolve rest-to-rest under double beams conflicts

Don't compute collisions if invisible or autoplace disabled

Improved interaction with user offsets

Implemented dedicated logic for multi-voice merged rests

Ignore rest offsets from old versions
@mike-spa
Vtest changes and additions
101e5a9
@mike-spa
Fix utests
7c08a8d
@mike-spa
Exposing default rest offset to Style dialog
20fd5c1
@mike-spa
Performance improvement
893ebbd
@mike-spa mike-spa force-pushed the verticalRestPositioning branch from 716ea0a to 893ebbd Compare March 22, 2023 08:15
abariska
abariska approved these changes Mar 22, 2023
View reviewed changes
Copy link

@abariska abariska left a comment

Choose a reason for hiding this comment

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

Performance feels good now. Difference between master and the PR is no more than +-1 sec (when talking about a processes which take ~50sec)

@mike-spa
Copy link
Contributor Author

mike-spa commented Mar 22, 2023 •
edited
Loading

Update: I've added a tiny (2 LOC) commit at the end cause I realized it was an obvious fix for #14650

@RomanPudashkin RomanPudashkin merged commit 484fc14 into musescore:master Mar 22, 2023
asattely added a commit to asattely/MuseScore that referenced this pull request Mar 22, 2023
@asattely
move BeamLayout to its own file
bebc3d7 
and rebase shenanigans for PR musescore#16719
asattely added a commit to asattely/MuseScore that referenced this pull request Mar 22, 2023
@asattely
move BeamLayout to its own file
9cf89c5 
and rebase shenanigans for PR musescore#16719
asattely added a commit to asattely/MuseScore that referenced this pull request Mar 22, 2023
@asattely
move BeamLayout to its own file
2cc6de2 
and rebase shenanigans for PR musescore#16719
@its-not-nice its-not-nice mentioned this pull request May 16, 2023
Closed
@mike-spa mike-spa mentioned this pull request Jun 12, 2023
Merged
@its-not-nice its-not-nice mentioned this pull request Jun 15, 2023
Closed
@mike-spa mike-spa deleted the verticalRestPositioning branch February 17, 2025 10:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@its-not-nice its-not-nice its-not-nice approved these changes

@RomanPudashkin RomanPudashkin RomanPudashkin approved these changes

@abariska abariska abariska approved these changes

Assignees
No one assigned
Labels
vtests This PR produces approved changes to vtest results
Projects
None yet
Milestone
No milestone
4 participants
@mike-spa @abariska @its-not-nice @RomanPudashkin