Harp pedalling diagrams and harp notation palette #12269

miiizen · 2022-07-04T21:15:20Z

This pull request adds harp pedalling diagrams to MuseScore for GSoC 2022.
These diagrams will be editable through a UI which will be built in the coming weeks.

Known issues

Save in score
Inspector
Style properties
Style (diagram size, placement) is not final and will be amended.
Text diagrams are not finished. A start has been made.
Screenreader not yet working
Some issues with internal xml representation causing loss of data (eg. between palette and score) - input on this very welcome!
Harp diagram not using the Leland font when it is set in styledef.cpp and the style menu in the program

I signed CLA
I made sure the code in the PR follows the coding rules
I made sure the code compiles on my machine
I made sure there are no unnecessary changes in the code
I made sure the title of the PR reflects the core meaning of the issue you are solving
I made sure the commit message(s) contain a description and answer the question "Why do those changes fix that particular issue?" or "Why are those changes really necessary as improvements?"
I made sure the commit message title starts with "fix #424242:" if there is a related issue
I created the test (mtest, vtest, script test) to verify the changes I made

src/engraving/libmscore/harppedaldiagram.cpp

RomanPudashkin · 2022-07-07T12:22:52Z

src/engraving/libmscore/harppedaldiagram.cpp

+    setXmlText(diagram);
+}
+
+const String HarpPedalDiagram::getStringName(HarpString str)


This method should be static and should be declared directly in this file. Also, HarpString should be renamed to HarpStringType (and the argument to "type" accordingly). After these manipulations the method itself should be renamed to harpStringTypeToString

src/engraving/libmscore/harppedaldiagram.cpp

src/engraving/libmscore/harppedaldiagram.h

src/palette/internal/palettecreator.cpp

cbjeukendrup · 2022-07-07T13:08:59Z

src/engraving/style/styledef.cpp

@@ -788,6 +788,24 @@ const std::array<StyleDef::StyleValue, size_t(Sid::STYLES)> StyleDef::styleValue
    { Sid::stringNumberFrameBgColor,      "stringNumberFrameBgColor",      PropertyValue::fromValue(draw::Color::transparent) },
    { Sid::stringNumberOffset,            "stringNumberOffset",            PointF(0.0, 0.0) },

+    { Sid::harpPedalDiagramFontFace,          "harpPedalDiagramFontFace",          "Leland" },


This probably should not be Leland, since <sym> symbols will automatically use the "Text" variant of the currently selected score font (e.g. Leland Text).

Also consider using these symbols for the accidentals (they will be a better size): https://www.w3.org/2019/03/smufl13/tables/standard-accidentals-for-chord-symbols.html

cbjeukendrup · 2022-07-07T13:55:25Z

src/engraving/libmscore/harppedaldiagram.cpp

+                    diagram.append(strName + String("<sym>accidentalNatural</sym>, "));
+                    break;
+                case PedalPosition::SHARP:
+                    diagram.append(strName + String("<sym>accidentalSharp</sym>, "));


I believe it is not usual to include a comma. Also, from what I can find on the internet, it is common to put D, C, and B on a second line. This is from Dorico:

@oktophonie Can you confirm that this is desirable?

The text diagrams in the design I'm following from @Tantacrul are only supposed to show the changes over previous settings, but yes the above is what I'd expect as a sort of initial state.
I'm still working on the text diagrams so can change this if need be!

Yes, showing only changes is good, but even then Dorico puts D,C,B on a separate line. I'm wondering if we should do that too.

@oktophonie - thoughts about having two lines?

Two lines is standard, as arranged in the Dorico example, and there should be no commas.

(One line is possible (D C B E F G A) but I've very rarely seen this - it would usually be just to save space in particularly tight situations.)

Harp pedal diagrams can now be added to score.

miiizen · 2022-07-12T17:43:44Z

Both types of pedal diagrams are now finished. The UI to edit them can be submitted as a separate PR or added to this one.
I'll squash and rebase later in case there's much to change.
Thanks!

cbjeukendrup · 2022-07-29T14:44:15Z

src/engraving/layout/layoutsystem.cpp

@@ -1209,6 +1209,18 @@ void LayoutSystem::layoutSystemElements(const LayoutOptions& options, LayoutCont
            }
        }
    }
+
+    //-------------------------------------------------------------
+    // Harp pedal diagrams


By putting this at the bottom of the function, harp pedal diagrams will be stacked 'outside' of everything else. For example, if they are above the staff, they will appear even above tempo markings and rehearsal marks. @oktophonie What would be the correct stacking order?

@miiizen, as suggested by @oktophonie, let's place them for now "outside" ottavas but "inside" anything else. That would mean moving this block of code to just above

MuseScore/src/engraving/layout/layoutsystem.cpp

Lines 1090 to 1094 in 3ee3c7e

//

// We need to known if we have FretDiagrams in the system to decide when to layout the Harmonies

//

bool hasFretDiagram = false;

oktophonie · 2022-07-30T09:10:40Z

Like other indications of playing technique I'd expect them to stay close to the stave without too much getting in between. This is pretty easy if they are between the staves or below the bottom one. For placing them above, someone with access to a lot of solo harp material might better advise. I'd expect them to go outside of 8va markings but inside anything else(?)

cbjeukendrup · 2022-08-23T23:47:24Z

src/engraving/libmscore/chordrest.cpp

@@ -522,6 +524,9 @@ EngravingItem* ChordRest::drop(EditData& data)
            if (fromPalette) {
                r->setXmlText(score()->createRehearsalMarkText(r));
            }
+        } else if (e->isHarpPedalDiagram()) {
+            HarpPedalDiagram* h = toHarpPedalDiagram(e);
+            h->updateDiagramText();


You could consider overriding the added() method in HarpPedalDiagram and then moving the call to updateDiagramText(); to the added() method.

cbjeukendrup · 2022-08-24T00:02:07Z

src/engraving/libmscore/harppedaldiagram.h

+
+class HarpPedalDiagram final : public TextBase
+{
+    std::vector<PedalPosition> _pedalState;


Since the number of entries is/should always be 7, how about using std::array<PedalPosition, 7>?

cbjeukendrup · 2022-08-24T00:11:15Z

src/engraving/libmscore/harppedaldiagram.cpp

+        static std::vector<std::pair<String, String> > symbolReplacements {
+            { u"\ued60", u"b " },
+            { u"\ued61", u" nat " },
+            { u"\ued62", u"# " }


I think these things will need to be translatable. You could drop this method in this PR and do it in #12597 instead. Let's ask @shoogle to advise about what the description should be exactly.

cbjeukendrup · 2022-08-24T00:19:43Z

src/engraving/libmscore/harppedaldiagram.cpp

+    case HarpStringType::A:
+        return String("A");
+    case HarpStringType::B:
+        return String("B");


We may need to take into account German note names. @oktophonie what do you think?

If yes, I'm not completely sure how to do that. We could do this through the 'normal' translation system that is also used for the UI, but that means that scores will look different if you open them in a MuseScore with a different language, which seems a bit dangerous to me. ISTR that we have some code somewhere to compute German note names, but I don't remember where and how it should be used.

cbjeukendrup · 2022-08-24T00:20:49Z

src/engraving/libmscore/harppedaldiagram.cpp

+using namespace mu::engraving;
+
+// Position of separator character in diagram
+static const int SEPARATOR_IDX = 3;


Could be constexpr instead of just const

cbjeukendrup · 2022-08-24T00:42:10Z

src/engraving/libmscore/harppedaldiagram.h

+using namespace mu::engraving;
+
+namespace mu::engraving {
+enum class PedalPosition {


You could write enum class PedalPosition : char {, to specify that this enum only needs the space of a char (1 byte). Same for HarpStringType. Tiny memory usage optimization.

cbjeukendrup · 2022-08-24T00:59:16Z

src/engraving/libmscore/harppedaldiagram.cpp

+        for (int idx = 0; idx < _pedalState.size(); idx++) {
+            if (idx == SEPARATOR_IDX) {
+                // insert separator
+                diagram.append(String("\ue683"));


Is there are reason to not use
<sym>harpPedalDivider</sym>
<sym>harpPedalRaised</sym>
<sym>harpPedalCentered</sym>
<sym>harpPedalLowered</sym>
respectively instead of the unicode values? That way, Leland should be used automatically as the font for the symbols. See also https://github.com/musescore/MuseScore/pull/12269/files#r915853649

(We might need to make sure here too that we're using Leland and not Leland Text; see

MuseScore/src/engraving/libmscore/textbase.cpp

Line 800 in 3ee3c7e

if (t->isDynamic() || t->textStyleType() == TextStyleType::OTTAVA) {

.)

cbjeukendrup · 2022-08-24T01:06:16Z

src/palette/internal/palettecell.cpp

@@ -99,6 +99,7 @@ const char* PaletteCell::translationContext() const
    case ElementType::BREATH:
    case ElementType::FERMATA:
    case ElementType::SYMBOL:
+    case ElementType::HARP_DIAGRAM:


For ElementType::HARP_DIAGRAM, this method should return "palette" (it should correspond with:

sp->appendElement(pedalDiagram, QT_TRANSLATE_NOOP("palette", "Harp pedal diagram")); ^~~~~~~

and

sp->appendElement(pedalTextDiagram, QT_TRANSLATE_NOOP("palette", "Harp pedal text")); ^~~~~~~

cbjeukendrup · 2022-08-24T01:11:06Z

src/engraving/libmscore/factory.cpp

@@ -163,6 +164,7 @@ static const ElementName elementNames[] = {
    { ElementType::STAFFTYPE_CHANGE,     "StaffTypeChange",      QT_TRANSLATE_NOOP("elementName", "Staff type change") },
    { ElementType::HARMONY,              "Harmony",              QT_TRANSLATE_NOOP("elementName", "Chord symbol") },
    { ElementType::FRET_DIAGRAM,         "FretDiagram",          QT_TRANSLATE_NOOP("elementName", "Fretboard diagram") },
+    { ElementType::HARP_DIAGRAM,         "HarpPedalDiagram",          QT_TRANSLATE_NOOP("elementName", "Harp Pedal Diagram") },


Minor thing: for ui texts, we use "Sentence case" instead of "Title Case". So this one would be "Harp pedal diagram".

cbjeukendrup · 2022-08-24T01:11:45Z

src/palette/internal/palettecreator.cpp

+
+    auto pedalTextDiagram = Factory::makeHarpPedalDiagram(gpaletteScore->dummy()->segment());
+    pedalTextDiagram->setIsDiagram(false);
+    pedalTextDiagram->updateDiagramText();


You could also consider to call updateDiagramText() in layout(), so that you don't need to remember to call it and so that it will always be up-to-date when other diagrams earlier in the score are modified.

(If you decide to do so, then my comment in src/engraving/libmscore/chordrest.cpp doesn't apply anymore)

cbjeukendrup · 2022-08-24T01:16:03Z

A rebase is needed. It would have my preference to merge this PR first, so that we can review the new changes in #12597 in isolation.

cbjeukendrup · 2022-08-24T01:26:18Z

src/engraving/libmscore/harppedaldiagram.cpp

+    case HarpStringType::G:
+        return String("G");
+    }
+    return String("");


Better just String() instead of String("") (in all places where an empty String is created).

Also, those String literals should be prefixed with u. So: String(u"A") or simply u"A".

Otherwise, you are converting from const char* to QString and then from QString to String. That's undesired because we want that the engraving module doesn't depend on Qt.

cbjeukendrup · 2022-08-24T01:52:17Z

Some quick tips for rebasing, in case it's useful:

About the #includes and forward declarations, I have recently done a PR to sort those alphabetically. This gives somewhat strange conflicts; to solve them, just choose the change from the master branch (or maybe Git will call it "Updated upstream") and then re-insert the #include that you inserted, please in the correct alphabetical order.
static const ElementName elementNames[] has been moved to typesconv.cpp
ElementType has been moved to types/types.h as opposed to libmscore/types.h
QT_TRANSLATE_NOOP has been replaced with TranslatableString (in many cases);
In PaletteCreator.cpp, some other new palettes have been added; you can simply preserve both changes

So in this case, the best strategy is often to choose the changes from the master branch and then reapply your additions. Of course, during rebasing, you can still see your changes (as they were before the rebase) here on GitHub, which you can use as a reference.

Finally: you could save the SHA of your last commit, 340ad21, so that if it goes completely wrong you can do git checkout 340ad21e138dc486a9294bd6e3e1fd838decdbd7 to go back to the state before rebase.

Jojo-Schmitz · 2022-08-25T16:15:45Z

Huh?

miiizen · 2022-08-25T16:24:08Z

Sorry for the confusion, please see #12597!

Jojo-Schmitz · 2022-08-25T16:47:38Z

But how does that match with #12269 (comment) ?

cbjeukendrup · 2022-08-25T16:49:16Z

@Jojo-Schmitz We changed the plans a bit; it is not very convenient for @miiizen to continuously maintain two PRs. We decided to move on with #12597.

Created base class for harp pedal diagram

a01f9b5

miiizen force-pushed the harp-pedalling branch from 8b35630 to 795abb9 Compare July 6, 2022 12:01

RomanPudashkin reviewed Jul 7, 2022

View reviewed changes

src/engraving/libmscore/harppedaldiagram.cpp Outdated Show resolved Hide resolved