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

Updated implementation of the Skyline class #23035

Merged
merged 3 commits into from
May 30, 2024

Conversation

mike-spa
Copy link
Contributor

@mike-spa mike-spa commented May 30, 2024

Skylines are the main tool that we use for the vertical spacing calculations. The current implementation is quite primitive in that the Skyline is represented as a mere geometrical set of lines. This is fine for some stuff, but has clear limitations:

  • We see the geometrical lines, but we don't know what items are underneath them, which prevents us from doing anything smart about it (e.g. a vertical padding table, which is something we desperately need).
  • We only ever see the line corresponding to the outermost edges, so we lose information about the inner items, which can be important (e.g. to decide kerning).
  • We can't manipulate the Skyline itself. For instance, we can't say "remove this item from the skyline" once it's been added as no information about the item itself has been retained. Nor can we adjust the Skyline if the item has been moved. One can only recreate the entire Skyline from scratch.

This update was long overdue. The main idea is that we already have the Shape class which is very powerful and can already do all of the above. So I've modified the internal implementation of the Skyline class (the public interface has stayed almost unchanged) to use the Shape class, which incidentally has made the Skyline class itself much simpler, as now most methods just wrap the corresponding Shape methods.

I've tested the performance hit on the biggest file I have and it is almost non measurable, in the order of 0.5%. All the vtest differences are either due to rounding differences or to improvements in the stave-centering logic.

Old skylines:
image

New skylines:
image

@MarcSabatella
Copy link
Contributor

Brilliant! FWIW, Werner’s original autoplace implementation was something more shape-based as well, but much more complicated as I recall, and in the end he scrapped it and replaced it by skyline. Combining the two concepts is so natural in hindsight! Looking forward what new possibilities arise…

@mike-spa mike-spa merged commit ca4e953 into musescore:master May 30, 2024
10 of 11 checks passed
@oktophonie oktophonie removed their request for review May 30, 2024 17:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants