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 #299644: add support for fret diagram rotation #5744

Merged

Conversation

jthistle
Copy link
Contributor

@jthistle jthistle commented Feb 21, 2020

resolves https://musescore.org/en/node/299644

This is what it looks like:

image

(rotated, rotated, normal, rotated with fret number)

Other changes in this PR:

  • a new property, ORIENTATION has been added. This can be either HORIZONTAL or VERTICAL.
  • cross and circle markers (above the diagram) have been made to look nicer, using a pen to draw them rather than using a font with the letters 'X' and 'O'
  • layout has been modified so that the diagrams are actually centred over the notehead/rest they are above, rather than using a hacky method that didn't change with the number of strings
  • a missing method for reading a CHANGE_METHOD type property

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Feb 21, 2020

What about rotating into the other direction, for left handed players?

HORIZONTAL_270 and HORIZONTAL_90 maybe, or HORIZONTAL and HORIZONTAL_LEFTHANDED?

@jthistle
Copy link
Contributor Author

damn, I hadn't thought of that... has that ever been requested?

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Feb 21, 2020

Not sure, but I believe so. But even if not, I see it coming

Edit: yes it has, see https://musescore.org/en/node/136436

@lasconic
Copy link
Contributor

Maybe it's an opportunity to use smufl glyphs and so make the markers configurable via the score fonts instead of hard coding the drawing with primitives? See https://www.smufl.org/version/latest/range/chordDiagrams/

@Jojo-Schmitz
Copy link
Contributor

Jojo-Schmitz commented Feb 22, 2020

Those seem to be too restrictive, like "Fingered fret (filled circle)" won't allow for fingering inside the dots and no barré and the fret diagrams have only 3-6 strings. The symbols for "Open string (O)" and "String not played (X)" might work though.

@jthistle
Copy link
Contributor Author

It's definitely an idea... could you explain the benefit of it over drawing, though? Because it wouldn't exactly be easy to implement.

@dmitrio95 dmitrio95 added the strings Affects translatable strings label Feb 28, 2020
@Harmoniker1
Copy link
Contributor

@jthistle Rebase needed

@anatoly-os
Copy link
Contributor

@jthistle ping :)

@jthistle
Copy link
Contributor Author

ah yes, will do

@anatoly-os
Copy link
Contributor

@jthistle ping :)

@anatoly-os anatoly-os added the work in progress not finished work or not addressed review label Apr 29, 2020
@jthistle
Copy link
Contributor Author

sorry sorry!

@jthistle
Copy link
Contributor Author

@anatoly-os just building now - I'll push once I'm sure it's still working correctly.

@jthistle jthistle force-pushed the 299644-fretboard-diagram-rotation branch from 8b82c50 to 5a41e84 Compare April 29, 2020 14:45
@jthistle
Copy link
Contributor Author

@anatoly-os rebased

@jthistle jthistle force-pushed the 299644-fretboard-diagram-rotation branch from 5a41e84 to 3e86e5f Compare April 29, 2020 15:19
@jthistle
Copy link
Contributor Author

jthistle commented Apr 29, 2020

vtests should be fixed now

edit: or not

@AntonioBL
Copy link
Contributor

@jthistle : for what concerns vtests, both PR vtests and ref vtests are now generated "on the fly", so that the vtests for one PR are always compared to the vtests of the parent branch (some sort of "differential" check).
If a PR introduces changes (even for the better, as in this case), the vtests will fail (I couldn't find a way for an "allowed failure" in github actions, which are used to do the vtests, or automatically post a comment in the PR). So a vtest "failure" is not necessarily a real failure; it just asks to carefully check what has changed, so that no regressions are introduced.
No need to change local refs or vtest scripts (unless you are adding a new vtest).

In this case, the difference is that "X" and "O" symbols in the fret diagram are a little bigger, for a better readability.

@anatoly-os anatoly-os merged commit dbfb281 into musescore:master May 2, 2020
@anatoly-os anatoly-os removed the work in progress not finished work or not addressed review label May 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
strings Affects translatable strings
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants