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

fix #277565: Shortening notes too much leads to bad stem layout #4086

Merged
merged 1 commit into from
Nov 21, 2018

Conversation

mattmcclinch
Copy link
Contributor

See https://musescore.org/en/node/277565.

Currently, the stem is only lengthened for 3, 4, or 5 hooks. This patch allows the stem to be lengthened for any number of hooks.

This patch also fixes the glyphs for 128th note flags in the mscore font so that they will align properly with the stem.

A few explanations about these changes:

  • At this point in the code, the _hook property is not a reliable indication of whether or not the chord will have hooks. It is better to use hookIdx.
  • The tab variable should only be non-NULL if the staff is in fact a TAB staff.
  • Stems outside the staff should not be shortened so much that the hook overshoots the notehead. See Behind Bars, page 16.

@jods4
Copy link

jods4 commented Nov 11, 2018

You describe this patch as handling 3+ hooks.
I experience the same issue with only a single hook on downwards stems.
Does your patch address that?
image

@mattmcclinch
Copy link
Contributor Author

Yes, my patch addresses that as well. It is what I meant when I was talking about stems outside the staff being shortened so much that the stem overshoots the notehead. I believe I described my patch as handling any number of hooks.

@@ -1214,8 +1255,8 @@ qreal Chord::defaultStemLength()
const Staff* st = staff();
qreal lineDistance = st ? st->lineDistance(tick()) : 1.0;

const StaffType* tab = st ? st->staffType(tick()) : 0;
if (tab && tab->isTabStaff()) {
const StaffType* tab = (st && st->isTabStaff(tick())) ? st->staffType(tick()) : 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

nullptr instead of 0?


qreal normalStemLen = small() ? 2.5 : 3.5;
switch (hookIdx) {
case 3: normalStemLen += small() ? .5 : 0.75; break; //32nd notes
Copy link
Contributor

Choose a reason for hiding this comment

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

We lost the comments describing magic numbers. Could you please add more info in the comments to hookAdjustment() method to describe magic numbers and calculations in general?

@jods4
Copy link

jods4 commented Nov 11, 2018

@mattmcclinch cool thanks!

Maybe also related? Downwards stems without hooks are sometimes very short on note groups.
image

This is very apparent on this screencap for two reasons:

  1. First downward stem is shorter than the previous beam group, although the lowest note in group is lower than previous group!!
  2. Last downward stem is really short, esp. compared to next upward stem.

@mattmcclinch
Copy link
Contributor Author

I am not a fan of stem shortening to begin with, but since Elaine Gould writes about it in Behind Bars (pg. 14) we have it in MuseScore. But it is a style setting that can be adjusted somewhat or even turned off completely on a per-score basis. Stems will not be shortened if the chord has a beam, which is why the beamed chords have longer stems than the non-beamed chords. Stem shortening only affects stems outside the staff, which is why the downstem chords in the picture have shorter stems than the upstem chords.

@jods4
Copy link

jods4 commented Nov 11, 2018

@mattmcclinch Thanks for the explanation! I don't like it either, so I'll make it more subte or maybe turn it off.

@lasconic
Copy link
Contributor

It could be worth adding a couple of vtests for this one and make sure it works for all supported fonts.

@anatoly-os
Copy link
Contributor

I will add the tests.

@anatoly-os anatoly-os merged commit 7ca06db into musescore:master Nov 21, 2018
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