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

fingering layout #4591

Merged
merged 1 commit into from Jan 29, 2019
Merged

fingering layout #4591

merged 1 commit into from Jan 29, 2019

Conversation

MarcSabatella
Copy link
Contributor

There are quite a few issues involving fingering layout. Some are design limitations, some are bugs inherited from 2.3.2, some are some regressions. This PR tries to address them all as well as possible.

Some of the issues fixed:

  • All sources agree that piano fingerings for both single notes and chords should be stacked above the chord, but we have always placed them to the left of noteheads because it was too hard to stack them correctly. Fingerings for single notes we place above the chord but not necessarily above the staff as we should. Pianists have relied on a plugin to fix the fingering, but that's not an option right now.
  • 2.3.2 did a good job of placing piano fingerings above beamed groups of notes, but 3.0 gets this wrong, overlapping the stem in many cases.
  • In 2.3.2, we placed LH guitar fingering to the left of notes as per the usual standard, but 3.0 only does this for chords, not single notes (we use the same algorithm for LH guitar fingering as for piano). Guitar players have rightly complained about this.
  • User styles would be useful to set custom offsets from notes, but that works only if autoplace leaves these in the default position.

No doubt there is still room for improvement even with my fixes, but I think mostly it will be about small tweaks - quibbling over exactly how far from the notehead the fingering is, etc. I'd like to see this merged and let people like cadiz1 and geetar have at it to provide more feedback.

One limitation I know we will probably want to revisit is the vertical alignment and spacing of LH guitar fingering in chords. With the possibility of tightly-spaced chords (including seconds), plus accidentals, it's going to be next to impossible to come up with perfect layout by default in all cases. My algorithm here does pretty well for many chords but will still have collisions in some cases. That's probably unavoidable, hopefully people find my approach avoids the need for manual adjustments in many cases and provides a good starting point in the rest.

Here's a sample:

image

@MarcSabatella
Copy link
Contributor Author

The broken tests are kind of to be expected as a side effect of how I implemented the algorithm, making use of the placement property. I could update the references, but frankly, I'd rather update the writing of Fingering elements to skip writing this property, because it is not actually needed (it is generated automatically). Hmm, I was thinking that would mean creating an override for Fingering::writeProperties, but actually, maybe I can avoid that by forcing this property to always be "styled" (it is not exposed in the UI). Anyhow, one way or another I will fix.

@MarcSabatella
Copy link
Contributor Author

FWIW, I updated the references. That way if we ever do decide to expose the placement property for fingering, the tests won't break again.

@MarcSabatella
Copy link
Contributor Author

MarcSabatella commented Jan 17, 2019

Updated to incorporate some excellent feedback from cadiz1 and geetar in the issue report https://musescore.org/en/node/280807

Speaking of issue reports, there's actually a whole bunch of them that are addressed by this, enough that it was not convenient to list them all in the commit message. When (!) this PR gets merged, I am happy to close issues manually.

}
}
for (Fingering* f : alignNote) {
if (f->autoplace())
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed if you already checked the elements earlier for autoplace? Or does autoplace have a side effect?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Harmless, but I can remove 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.

(fixed)

@MarcSabatella MarcSabatella force-pushed the left-fingering branch 2 times, most recently from 6006860 to bf677b4 Compare January 26, 2019 18:10
@MarcSabatella
Copy link
Contributor Author

Build failure doesn't seem to be my fault, AppVeyor may be full?

@Jojo-Schmitz
Copy link
Contributor

Force a new build and it should work

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants