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

Narrow spacing improvements #15174

Merged
merged 6 commits into from Mar 23, 2023

Conversation

mike-spa
Copy link
Contributor

@mike-spa mike-spa commented Dec 14, 2022

Narrow spacing improvements (and a few related things).

Before:

before.mp4

After:

after.mp4

@mike-spa mike-spa marked this pull request as ready for review January 30, 2023 12:53
@oktophonie oktophonie added the P1 Priority: High label Feb 16, 2023
@mike-spa mike-spa force-pushed the narrowSpacingIMprovements branch 4 times, most recently from 5571da5 to e53324f Compare February 24, 2023 13:07
@cbjeukendrup cbjeukendrup added this to In Progress in MuseScore 4.1 Feb 25, 2023
@oktophonie oktophonie added the vtests This PR produces approved changes to vtest results label Feb 27, 2023
@abariska abariska self-requested a review March 17, 2023 13:57
@mike-spa
Copy link
Contributor Author

@RomanPudashkin yeah, I know it's extremely inefficient, but it's the only way to squeeze everything while still producing an acceptable engraving result. The good news is that LayoutSystem::manageNarrowSpacing() will never be called by the default layout. The conditions for calling this function only happen if the user specifically requires to fit in the system more measures than can normally fit (again, also quite rare). In fact, I've tried the usual Beethoven 9 score and this function never gets called, so the final layout time is even slightly better than current master:

Master:
master

PR:
thisPR

Besides that, I've removed the calls to minSysTicks and maxSysTicks cause I've realized that the values could be passed down by the caller. And I've added comments on the constants.

@RomanPudashkin
Copy link
Contributor

@mike-spa The vtests check hangs up on this PR. Please take a look at it

@RomanPudashkin RomanPudashkin self-requested a review March 21, 2023 08:25
@mike-spa mike-spa force-pushed the narrowSpacingIMprovements branch 2 times, most recently from 4920c15 to 590b080 Compare March 21, 2023 10:05
@mike-spa
Copy link
Contributor Author

Ok, I don't understand why the vtests seem to get stuck: I can run them locally on my machine (Windows) and they run fine. I'll try on my Linux machine and see if something changes.

@mike-spa
Copy link
Contributor Author

Update on vtests: they run perfectly fine both on my Windows machine and on my Linux machine. So I really don't know what to do with them here

@igorkorsukov
Copy link
Contributor

@mike-spa Please, disable (comment out) the drawdata steps so they don't block you (they are in alpha mode now), I'll see what's wrong later https://github.com/musescore/MuseScore/blob/master/.github/workflows/ci_vtests.yml#L153

mike-spa and others added 6 commits March 23, 2023 09:40
Clefs are extremely annoying, because they are literally the only musical item that is spaced right-to-left, while the whole system is designed to space left-to-right. Previously, I had kind of hacked into it. The only real solution, though, is to split the spacing in 2 passes. Pass 1: place all the left-aligned segments. Pass 2: place all the right-aligned segments (basically, clefs and breaths).
correction
Otherwise what's the point of noBreak...
resolve conflict
@mike-spa
Copy link
Contributor Author

Ok, I've removed the problematic bit from the vtests and rebased it, let's hope it works 👍 (btw the current master crashes on startup)

@RomanPudashkin RomanPudashkin merged commit 55d4dd1 into musescore:master Mar 23, 2023
10 of 11 checks passed
@oMrSmith
Copy link

The conditions for calling this function only happen if the user specifically requires to fit in the system more measures than can normally fit (again, also quite rare).

Being able to force measures into a system is a heavy new feature! Thanks for introducing this into MuseScore.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engraving P1 Priority: High vtests This PR produces approved changes to vtest results
Projects
Status: Done
MuseScore 4.1
  
In Progress
Development

Successfully merging this pull request may close these issues.

None yet

6 participants