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

Segment-based horizontal spacing #25099

Merged
merged 5 commits into from
Oct 21, 2024
Merged

Segment-based horizontal spacing #25099

merged 5 commits into from
Oct 21, 2024

Conversation

mike-spa
Copy link
Contributor

@mike-spa mike-spa commented Oct 9, 2024

Resolves: #17423
Resolves: #23507 (no idea what caused this, but it appears that among the many changes I've made this issue has gone away)

Musescore's horizontal spacing algorithms have always worked on a strictly measure-by-measure basis. Essentially, each measure is treated as an individual block which computes its own internal spacing (the main calculations taking place in MeasureLayout::computeWidth) , and systems are formed by adding these blocks one after the other. My original spacing improvements worked around this issue in many different ways, but they didn't change the fundamental fact that measures are still spaced separately. This was always going to be inadequate for our future needs, but the immediate necessity is that it makes it impossible to let lyrics cross bar lines, because we can't check for collisions beyond measure boundaries.

This PR implements a Segment-based (as opposed to measure-based) horizontal spacing approach. The core calculations now happen in HorizontalSpacing::spaceSegments, where segments are spaced in system-coordinates as if measures didn't exist, and then we use the resulting segment positions to establish measures widths and positions in HorizontalSpacing::setPositionsAndWidths.

The new system supports all the previous measure-based functionality, including: the fact that systems are still filled one measure at a time, so also their width (and therefore the line-breaking decisions) are updated one measure at a time (this is why I have HorizontalSpacing::updateSpacingForLastAddedMeasure alongside HorizontalSpacing::computeSpacingForFullSystem); the fact that measures can have individual stretch applied; styled parameters such as minimum measure width; etc.

Rethinking the system as a whole allowed a series of further improvements.

  1. The calculations of spacing ratios have been strongly simplified and streamlined. This has performance benefits, but it also resolves a long-standing issue that we've had since my first spacing work, namely that the presence of short notes in a system causes the overall spacing to become wider. That doesn't happen anymore, and most of the v-test differences are due to this.
  2. The code got a big cleanup. Lots of old semi-redundant code has been removed, and I've removed methods and data fields from the Measure and Segment class that didn't belong there, and moved all the relevant parts to the HorizontalSpacing class.
  3. Thanks to HorizontalSpacing::stopCheckingPreviousSegments, we now make smarter decisions on how many segments we go back to look for collisions (before, we stopped at measure boundaries, but that's not relevant anymore). This strongly reduces the total number of segment-to-segment collision checks we do (from 340.000 to less than 100.000 in one big file I've tested), which also has significant performance benefits.
  4. Overall, the time for a full-layout has improved by about 15% on a big piano score (Alkan concerto) and by 4-5% on a on big symphonic score (Beethoven 9).

Before:
Screenshot 2024-10-04 174053

After:
Screenshot 2024-10-04 174149

@mike-spa mike-spa force-pushed the LYRICS branch 2 times, most recently from 009f6bb to 955469d Compare October 11, 2024 15:17
@oktophonie oktophonie added the vtests This PR produces approved changes to vtest results label Oct 15, 2024
return 0.0;
case SegmentType::ChordRest:
{
Shape leftBarrier(RectF(0.0, -0.5 * DBL_MAX, 0.0, DBL_MAX));
Copy link
Contributor

Choose a reason for hiding this comment

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

-0.5 * DBL_MAX

Could you explain the purpose of this? :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

image

It's a bit hacky, but it's also a simple way of ensuring that nothing can cross the left margin of the system (and it was done this way since forever). We basically compute minLeft against a "barrier" rectangle of infinite height (but our infinite is actually DBL_MAX, and if we want our rectangle to have height = DBL_MAX we let it start from -0.5 * DBL_MAX so it will extend to both large negative and large positive y).

@RomanPudashkin
Copy link
Contributor

@mike-spa great job! We should also do some performance tests (compare layout time on large scores). @DmitryArefiev FYI

@mike-spa
Copy link
Contributor Author

@RomanPudashkin @DmitryArefiev I've already done some performance testing using our built in profiler and the results show significant improvements (see PR description). It's probably not going to be noticeable by the end user, but it's still a good step

@mike-spa
Copy link
Contributor Author

@RomanPudashkin all done!

@mike-spa mike-spa merged commit 4dc75b3 into musescore:master Oct 21, 2024
10 of 11 checks passed
@peteranglea
Copy link

Wonderful! Been waiting a long time for this one. Can't wait to test it out. Thank you!

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
Development

Successfully merging this pull request may close these issues.

Staves/systems overflowing right margin Less-restrictive horizontal lyric spacing
4 participants