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

Improve laissez vib articulation placement #23392

Merged
merged 3 commits into from
Jul 10, 2024

Conversation

miiizen
Copy link
Contributor

@miiizen miiizen commented Jun 28, 2024

This PR improves the placement of laissez vib articulation symbols.
image

@miiizen miiizen changed the title Improve lasser vib articulation placement Improve lassez vib articulation placement Jun 28, 2024
@miiizen miiizen changed the title Improve lassez vib articulation placement Improve laissez vib articulation placement Jun 28, 2024
@Dima-S-Jr
Copy link

@miiizen, does this solve #16726 or #20919?

@oktophonie
Copy link
Contributor

oktophonie commented Jun 28, 2024

@Dima-S-Jr No.

@oktophonie oktophonie added the vtests This PR produces approved changes to vtest results label Jul 2, 2024
@oktophonie oktophonie requested a review from mike-spa July 2, 2024 11:00
@@ -385,6 +385,14 @@ void HorizontalSpacing::computeNotePadding(const Note* note, const EngravingItem
}
}

for (Articulation* a : note->chord()->articulations()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a bit hacky (and a bit of a performance concern as this is a very hot function). I think the better way to do this would be to add the shape of the LV articulation to its chord. That way, you can simply add to the padding table an Articulation->otherElement entry. At the moment, articulations are never added to segment shapes so LV will be effectively the only articulation item for which that entry is relevant (you can say that in a comment, there are a few other similar cases). In future, I'd like all articulations to be added to the chord shape (at the moment they aren't I think mostly because of skyline-related concerns), because wide articulations should be accounted for in horizontal spacing. When we do that, we'll think about how to specialize the padding for different articulation types

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorted - as articulations are layed out well after horizontal spacing is calculated, this required moving lv layout out of LayoutArticulations2 and to layoutPitched and layoutTablature.

Copy link
Contributor

@mike-spa mike-spa left a comment

Choose a reason for hiding this comment

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

Excellent job

@mike-spa mike-spa merged commit b4071bb into musescore:master Jul 10, 2024
10 of 11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
engraving vtests This PR produces approved changes to vtest results
Projects
Development

Successfully merging this pull request may close these issues.

4 participants