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

Smarter gliss positioning around chord shapes #19403

Merged
merged 2 commits into from
Oct 20, 2023

Conversation

miiizen
Copy link
Contributor

@miiizen miiizen commented Sep 14, 2023

Resolves: #17955
Resolves: #19456

Calculates the edge of chords at start/end of gliss using right/leftMostEdgeAtHeight allowing the lines to better avoid parts of the chord shape.

Screenshot 2023-10-17 at 10 10 12
  • I signed the CLA
  • The title of the PR describes the problem it addresses
  • Each commit's message describes its purpose and effects, and references the issue it resolves
  • If changes are extensive, there is a sequence of easily reviewable commits
  • The code in the PR follows the coding rules
  • There are no unnecessary changes
  • The code compiles and runs on my machine, preferably after each commit individually
  • I created a unit test or vtest to verify the changes I made (if applicable)

@miiizen miiizen changed the title Symbols outside notes do not offset gliss x Symbols outside notes do not offset gliss x position Sep 14, 2023
@miiizen miiizen marked this pull request as draft September 14, 2023 16:34
@miiizen miiizen marked this pull request as ready for review September 15, 2023 08:15
@oktophonie oktophonie added the vtests This PR produces approved changes to vtest results label Sep 15, 2023
// don't consider symbols above and below noteheads
double yMin = cr1->upNote()->y() + cr1->upNote()->headHeight();
double yMax = cr1->downNote()->y();
mu::remove_if(cr1shape, [yMin, yMax](ShapeElement& s) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
mu::remove_if(cr1shape, [yMin, yMax](ShapeElement& s) {
mu::remove_if(cr1shape, [yMin, yMax](const ShapeElement& s) {

Comment on lines 2635 to 2628
Shape cr2shape = cr2->shape();
double yMin2 = cr2->upNote()->y() + cr2->upNote()->headHeight();
double yMax2 = cr2->downNote()->y();
mu::remove_if(cr2shape, [yMin2, yMax2](ShapeElement& s) {
if (!s.toItem || (s.toItem->isSymbol() && (s.toItem->y() < yMin2 || s.toItem->y() > yMax2))) {
return true;
} else {
return false;
}
});
Copy link
Contributor

Choose a reason for hiding this comment

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

There is some code duplication here... perhaps it would be preferable to create a local lambda function and call that twice. I.e.

    auto relevantShape(const Chord* cr) -> Shape {
        Shape s = cr->shape();
        double yMin = …; double yMax = …; …
        mu::remove_if(…);
        return s;
    }
    …
    offs1.rx() += relevantShape(cr1).right() - anchor1->pos().x();

@cbjeukendrup
Copy link
Contributor

Actually it looks like it would be possible to use Michele's new rightMostEdgeAtHeight and leftMostEdgeAtHeight methods for this. That might be even better.

@cbjeukendrup
Copy link
Contributor

Probably also helps for #19456 (at least when using rightMostEdgeAtHeight and leftMostEdgeAtHeight, if that's indeed possible)

@cbjeukendrup
Copy link
Contributor

A rebase is needed

@miiizen miiizen changed the title Symbols outside notes do not offset gliss x position Smarter gliss positioning around chord shapes Oct 17, 2023
if (upDown != 0) {
// Only check top/bottom half of note depending on gliss approach direction
// to avoid clearing acidentals the line won't collide with
yAbove2 = upDown == 1 ? noteMiddle - 1 : yAbove2;
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it intentional that there is an "absolute" - 1 here? Shouldn't it be relative to spatium or line distance? (Or is it that already and am I missing that?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Apologies - this was to make sure the ledger line was included in the left most edge check. I've replaced it with the thickness of the ledger line

@RomanPudashkin RomanPudashkin merged commit 049d0ff into musescore:master Oct 20, 2023
10 of 11 checks passed
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.

Multiple glissando lines do not extend full distance between chords symbol above note displaces gliss line
4 participants