The #GreatAccidentalsReform #21042

mike-spa · 2024-01-16T17:53:58Z

Resolves: #9302

A complete rewrite of the accidental placement algorithms, making good use of the recently introduced accidental cutouts. Basically see the vtests.

MarcSabatella · 2024-01-16T19:29:56Z

Wow, quite the project! I look forward to checking out the results. I wanted to find my old test file based on Daniel Spreadbury's blog article https://blog.dorico.com/2014/03/development-diary-part-six, but unfortunately I can't seem to find the MSCZ file itself. I did find the results of one of my other tests from back then, and I'll be curious to compare:

mike-spa · 2024-01-17T08:38:28Z

Disclaimer: don't get overexcited over any particular details. This is just me not resisting the urge to use the new cutouts and trying out stuff.

Tantacrul · 2024-01-17T20:19:21Z

LOVING THIS

bkunda · 2024-01-18T13:23:36Z

Fabulous progress @mike-spa!

src/engraving/dom/accidental.cpp

src/engraving/dom/accidental.h

src/engraving/infrastructure/shape.h

RomanPudashkin · 2024-05-16T14:05:41Z

src/engraving/rendering/dev/accidentalslayout.cpp

+
+    m_spatium = accidental->spatium();
+
+    m_verticalAccidentalToAccidentalClearance = 0.15 * m_spatium;


magic numbers

still actual

I don't know how else should I write this? I'm just initializing constants that I will need later. Some of these constants have a style, others are hardcoded. Problem is I can't hardcode these as const or constexpr because they need to be constructed from other information when constructing AccidentalsLayoutContext data

You should specify (in a comment) how you found / why exactly you are using these constants

This 0.15 (and other magic numbers) is used in a lot of different places in this PR. Worth adding a named constexpr

I've removed all the magic numbers I had used around the file and collected them all here with proper names and a comment that explains that they are just engraving parameters that don't have a style. The fact that some of them have the same value (like 0.15) is just a coincidence, they actually all mean different things.

mike-spa · 2024-05-17T12:29:02Z

@RomanPudashkin done! 👍

RomanPudashkin · 2024-05-21T11:59:00Z

src/engraving/rendering/dev/accidentalslayout.cpp

+{
+    std::vector<Accidental*> accidentals;
+
+    for (Chord* chord : chords) {


Please use const with pointers unless you are going to change their value

RomanPudashkin · 2024-05-21T12:01:37Z

src/engraving/rendering/dev/accidentalslayout.cpp

+            if (!acc) {
+                continue;
+            }
+            acc->setPos(0.0, 0.0);


This is not ideal... This function not only collects accidentals but also changes some properties. Ideally, it should have only one responsibility (find and return accidentals)

RomanPudashkin · 2024-05-21T12:14:09Z

src/engraving/rendering/dev/accidentalslayout.cpp

+    verticallyAlignAccidentals(ctx);
+}
+
+void AccidentalsLayout::findOctaves(AccidentalsLayoutContext& ctx)


Since this code will be called quite often, it is probably better to combine this function with findSeconds. This way, we will reduce the complexity of the algorithm

RomanPudashkin · 2024-05-21T12:16:48Z

src/engraving/rendering/dev/accidentalslayout.cpp

+    firstGroup.push_back(ctx.allAccidentals.front());
+    ctx.accidentalSubChords.push_back(firstGroup);
+
+    for (int i = 1; i < ctx.allAccidentals.size(); ++i) {


This will generate a warning as size() returns size_t

RomanPudashkin · 2024-05-21T12:23:21Z

src/engraving/rendering/dev/accidentalslayout.cpp

+        TLayout::layoutAccidental(acc, acc->mutldata(), ctx.conf());
+    }
+
+    AccidentalsLayoutContext accidentalsLayoutContext(allAccidentals, chords);


It's better to use std::move with allAccidentals to avoid creating a copy (the AccidentalsLayoutContext constructor also needs to be adjusted)

RomanPudashkin · 2024-05-21T12:35:28Z

src/engraving/rendering/dev/accidentalslayout.cpp

+void AccidentalsLayout::mergeSubGroupsWithOctavesAcross(AccidentalsLayoutContext& ctx)
+{
+    bool foundOctave = false;
+    do {


I'm not sure what's going on in this function, but it's very complex (5 nested loops + lots of operations on std::vector). Is there any way to simplify/optimize it?

I've moved out some code to make it more readable, but unfortunately I can't think of any way to simplify this. These are very gnarly engraving requirements resulting in very gnarly code logic. Luckily this function (and the next one that you pointed out) is used rarely so it shouldn't impact performance

RomanPudashkin · 2024-05-21T12:42:22Z

src/engraving/rendering/dev/accidentalslayout.cpp

+    applyOrderingOffsets(accidentals);
+}
+
+std::vector<std::vector<Accidental*> > AccidentalsLayout::splitIntoPriorityGroups


It's better to add an alias for this type

RomanPudashkin · 2024-05-21T12:44:00Z

src/engraving/rendering/dev/accidentalslayout.cpp

+void AccidentalsLayout::moveOctavesToSecondGroup(std::vector<std::vector<Accidental*> >& subGroups)
+{
+    std::vector<Accidental*>& firstGroup = subGroups[0];
+    std::vector<Accidental*>& secondGroup = subGroups[1];


It looks like std::array might be a better type for subGroups

This is one particular case (with conditions defined by the caller), but in general subGroups may have different sizes

worth adding this

IF_ASSERT_FAILED(subGroups.size() == 2) {
return;
}

RomanPudashkin · 2024-05-21T12:56:12Z

src/engraving/rendering/dev/accidentalslayout.cpp

+    return 0.0;
+}
+
+double AccidentalsLayout::computePadding(Accidental* acc, const EngravingItem* chordElement, AccidentalsLayoutContext& ctx)


const AccidentalsLayoutContext&

RomanPudashkin · 2024-05-21T12:56:28Z

src/engraving/rendering/dev/accidentalslayout.cpp

+    AccidentalType accType = acc->accidentalType();
+    if (chordItem->isLedgerLine() && (accType == AccidentalType::NATURAL || accType == AccidentalType::SHARP)) {
+        if (chordItem->y() > accShape.top() && chordItem->y() < accShape.bottom()) {
+            return accShape.right() - chordElement.left() + 0.1 * ctx.spatium();


RomanPudashkin · 2024-05-21T13:35:27Z

src/engraving/rendering/dev/accidentalslayout.cpp

+            for (int j = 0; j < thisGroup.size(); ++j) {
+                Accidental* acc1 = thisGroup[j];
+                for (Accidental* secondAcc : acc1->ldata()->seconds.value()) {
+                    if (muse::remove(nextGroup, secondAcc)) {


same question. remove() isn't very efficient with std::vector

mike-spa · 2024-05-22T10:25:53Z

@RomanPudashkin done! 👍

RomanPudashkin · 2024-05-24T10:56:38Z

src/engraving/rendering/dev/accidentalslayout.cpp

+        TLayout::layoutAccidental(acc, acc->mutldata(), ctx.conf());
+    }
+
+    AccidentalsLayoutContext accidentalsLayoutContext(std::move(allAccidentals), std::move(chords));


std::move(chords) - this has no effect as chords is const std::vector<Chord*>& :)

RomanPudashkin · 2024-05-24T11:07:22Z

src/engraving/rendering/dev/accidentalslayout.cpp

+    return subGroups;
+}
+
+AccidentalGroups AccidentalsLayout::groupAccidentalsByNoteXPos(std::vector<Accidental*>& accidentals)


This vector should be const

mike-spa marked this pull request as draft January 17, 2024 08:26

igorkorsukov force-pushed the master branch 6 times, most recently from fa1f8d3 to 525a11a Compare February 14, 2024 09:08

mike-spa force-pushed the accidentalsReform branch from dd89567 to 63f35fc Compare April 2, 2024 10:30

mike-spa marked this pull request as ready for review April 2, 2024 10:30

musescore deleted a comment from sammik Apr 2, 2024

mike-spa requested a review from oktophonie April 2, 2024 10:44

oktophonie added the vtests This PR produces approved changes to vtest results label Apr 3, 2024

oktophonie approved these changes Apr 3, 2024

View reviewed changes

mike-spa requested review from cbjeukendrup and RomanPudashkin April 4, 2024 08:02

mike-spa force-pushed the accidentalsReform branch 2 times, most recently from 642a240 to 5777d2e Compare April 16, 2024 07:30

musescore deleted a comment from Jojo-Schmitz Apr 16, 2024

mike-spa force-pushed the accidentalsReform branch from 5777d2e to 7bfe8d1 Compare April 18, 2024 09:04

RomanPudashkin requested a review from Eism April 26, 2024 14:22

mike-spa force-pushed the accidentalsReform branch from 7bfe8d1 to 1ceb6a0 Compare May 2, 2024 08:41

mike-spa force-pushed the accidentalsReform branch 2 times, most recently from d5c2f0e to ec94049 Compare May 15, 2024 07:55

RomanPudashkin reviewed May 16, 2024

View reviewed changes

src/engraving/dom/accidental.cpp Outdated Show resolved Hide resolved

RomanPudashkin reviewed May 16, 2024

View reviewed changes

src/engraving/dom/accidental.h Outdated Show resolved Hide resolved

RomanPudashkin reviewed May 16, 2024

View reviewed changes

src/engraving/infrastructure/shape.h Outdated Show resolved Hide resolved

RomanPudashkin reviewed May 16, 2024

View reviewed changes

mike-spa force-pushed the accidentalsReform branch 2 times, most recently from ecee97a to e1b8bb6 Compare May 17, 2024 11:20

RomanPudashkin reviewed May 21, 2024

View reviewed changes

mike-spa force-pushed the accidentalsReform branch 2 times, most recently from adc9a8a to 6e0f341 Compare May 22, 2024 09:33

RomanPudashkin reviewed May 24, 2024

View reviewed changes

mike-spa added 3 commits May 24, 2024 13:41

New and updated vtests

e79b16b

Remove all legacy accidental code

49c7668

Introduce new accidental placement algorithm and options

3a859a6

mike-spa force-pushed the accidentalsReform branch from 6e0f341 to f27403f Compare May 24, 2024 11:41

Code review

f074b44

mike-spa force-pushed the accidentalsReform branch from f27403f to f074b44 Compare May 24, 2024 11:44

RomanPudashkin approved these changes May 24, 2024

View reviewed changes

oktophonie removed request for Eism and cbjeukendrup May 24, 2024 12:13

RomanPudashkin merged commit 6644d91 into musescore:master May 24, 2024
10 of 11 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

The #GreatAccidentalsReform #21042

The #GreatAccidentalsReform #21042

mike-spa commented Jan 16, 2024 •

edited

Loading

MarcSabatella commented Jan 16, 2024 •

edited

Loading

mike-spa commented Jan 17, 2024

Tantacrul commented Jan 17, 2024

bkunda commented Jan 18, 2024

RomanPudashkin May 16, 2024

RomanPudashkin May 21, 2024

mike-spa May 21, 2024

RomanPudashkin May 21, 2024

RomanPudashkin May 21, 2024

mike-spa May 22, 2024 •

edited

Loading

mike-spa commented May 17, 2024

RomanPudashkin May 21, 2024

RomanPudashkin May 21, 2024

RomanPudashkin May 21, 2024

RomanPudashkin May 21, 2024

RomanPudashkin May 21, 2024

RomanPudashkin May 21, 2024

mike-spa May 22, 2024

RomanPudashkin May 21, 2024

RomanPudashkin May 21, 2024

mike-spa May 22, 2024

RomanPudashkin May 24, 2024

RomanPudashkin May 21, 2024

RomanPudashkin May 21, 2024

RomanPudashkin May 21, 2024

mike-spa commented May 22, 2024

RomanPudashkin May 24, 2024

RomanPudashkin May 24, 2024


		m_spatium = accidental->spatium();

		m_verticalAccidentalToAccidentalClearance = 0.15 * m_spatium;

The #GreatAccidentalsReform #21042

The #GreatAccidentalsReform #21042

Conversation

mike-spa commented Jan 16, 2024 • edited Loading

MarcSabatella commented Jan 16, 2024 • edited Loading

mike-spa commented Jan 17, 2024

Tantacrul commented Jan 17, 2024

bkunda commented Jan 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-spa May 22, 2024 • edited Loading

Choose a reason for hiding this comment

mike-spa commented May 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-spa commented May 22, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mike-spa commented Jan 16, 2024 •

edited

Loading

MarcSabatella commented Jan 16, 2024 •

edited

Loading

mike-spa May 22, 2024 •

edited

Loading