Collisions overhaul (and other improvements to horizontal spacing) #11099

mike-spa · 2022-04-05T15:24:51Z

This PR rewrites a substantial part of horizontal spacing, particularly the way we perform collision checks. Most notably (and finally) now items can tuck (or kern) above/below each other.

In more detail, what I did is:

Removed the logic within the code that was preventing elements from kerning.
Implemented a system which calculates the spacing at "high resolution", meaning it considers every single graphical piece of an item (notehead, stem, hook, ledger lines, etc...).
Managed several exceptions and special cases related to kerning (basically, cases when items should not kern).
Refactored the interaction of all the user-defined spacing settings (specifically those from Style -> Measure).
Corrected a small error related to the spacing of polyrythms.
Corrected mid-system clefs positioning.
Implemented a smarter way of deciding how many measures fit in one system. (I had tried this already in previous PRs but it was breaking on edge cases. Now it is finally stable.)

A side benefit of allowing items to tuck is that we save a huge amount of horizontal space that we were previously wasting, which lets us:
8. Increase the default value of minNoteDistance to a much more reasonable 0.6 spaces.
This is done in a separate commit, as it requires to also change all template files.

Basically, every single visual test is expected to fail, and I've usually had also utests failing on beams, so that's also to be expected.

@MarcSabatella if I may bother you, it'd be great if you could link some of the old issues that will be fixed by this (one of which you mentioned already). Thanks for the amazing support!

Before:

After:

Before:

After:

MarcSabatella · 2022-04-06T03:29:16Z

This is fantastic! Happy to update the issues once this is merged. But, I did want to verify something. When I took a crck at fixing this a couple of years ago, the issue I ran into was that at the point where we were calculating the spacing requirements for a given note, we didn't yet know for sure if the previous note was upstem or downstem if it was beamed. That's because beam directions weren't computer until later. So in any case where the initially guessed stem direction was up, but the actual stem direction turned out to be down, we would make the wrong call regarding the tucking. See for instance my examples in #4784

I can totally believe this might not be an issue any \more because the beam code has been rewritten. But I wouldn't mind hearing confirmation.

ecstrema · 2022-04-06T04:29:05Z

src/engraving/layout/layoutsystem.cpp

-        // be squeezed. I'm temporarily rolling back the idea (still a bit too risky in some edge cases) [M.S.]
-        double acceptanceRange = 1;
-        bool doBreak = (system->measures().size() > 1) && ((curSysWidth + ww) > systemWidth * acceptanceRange);
+        double acceptanceRange = 0.3 * system->squeezableSpace();


Where does the 0.3 come from?

Nothing, it just looks good. May be worth a comment though, thanks.

ecstrema · 2022-04-06T04:30:24Z

src/engraving/layout/layoutmeasure.cpp

@@ -745,6 +745,26 @@ void LayoutMeasure::getNextMeasure(const LayoutOptions& options, LayoutContext&
    }

    LayoutBeams::createBeams(score, ctx, measure);
+    /* HACK: The real beam layout is computed at much later stage (you can't do the beams until you know


@MarcSabatella This may be the answer to your question

ecstrema · 2022-04-06T04:35:23Z

Just to be sure: are these vtests correct?

Lyrics 6-1

Here the note order changed. Is it expected?

changed file?

No lyrics under timesig:

(multiple of these)

weird

and

I have probably missed some more.

ecstrema · 2022-04-06T04:35:56Z

One tiny question: Is there any performance overhead for this more complex layout algorithm?

mike-spa · 2022-04-06T07:00:56Z

This is fantastic! Happy to update the issues once this is merged. But, I did want to verify something. When I took a crck at fixing this a couple of years ago, the issue I ran into was that at the point where we were calculating the spacing requirements for a given note, we didn't yet know for sure if the previous note was upstem or downstem if it was beamed. That's because beam directions weren't computer until later. So in any case where the initially guessed stem direction was up, but the actual stem direction turned out to be down, we would make the wrong call regarding the tucking. See for instance my examples in #4784

I can totally believe this might not be an issue any \more because the beam code has been rewritten. But I wouldn't mind hearing confirmation.

@MarcSabatella stems were a real head scratcher. It turned out that now direction was known at the point of horizontal spacing, but extension was not. And yes, @Marr11317 pointed out the answer. Not an elegant solution at all (and I may look for a better one in future) but one that seems to do the job.

mike-spa · 2022-04-06T07:26:04Z

@Marr11317 Clefs/timesigs/keysigs are an exception to kerning, meaning they shouldn't kern above/below other items. Lyrics are the exception to the exception to kerning. Gonna look into that.

ecstrema · 2022-04-11T18:02:07Z

src/engraving/libmscore/measure.cpp

@@ -4098,7 +4101,8 @@ void Measure::computeWidth(Segment* s, qreal x, bool isSystemHeader, Fraction mi
    bool first  = isFirstInSystem();
    const Shape ls(first ? RectF(0.0, -1000000.0, 0.0, 2000000.0) : RectF(0.0, 0.0, 0.0, spatium() * 4));

-    qreal minNoteSpace = score()->noteHeadWidth() + score()->styleMM(Sid::minNoteDistance);
+    static constexpr double spacingMultiplier = 1.2;
+    qreal minNoteSpace = score()->noteHeadWidth() + spacingMultiplier * score()->styleMM(Sid::minNoteDistance);


If this spacingMultiplier is only used there, why not modify Sid::minNoteDistance instead?

ecstrema · 2022-04-11T18:05:16Z

src/engraving/libmscore/score.cpp

@@ -5401,6 +5405,160 @@ void Score::doLayoutRange(const Fraction& st, const Fraction& et)
    }
 }

+void Score::createPaddingTable()
+{
+    double minimumPaddingUnit = 0.1 * spatium();


another magic number. Why not 0.01?

This is a hard limit on the distance that two items can come within; it only applies in certain special situations, e.g. polyrhythms between notes in two voices where the normal minNoteDistance does not apply. 0.01 would be too small, but like all the other settings this is to become user-configurable eventually.

ecstrema · 2022-04-11T18:06:04Z

src/engraving/libmscore/score.cpp

+        }
+    }
+
+    const double ledgerPad = 0.25 * spatium();


should'nt that ledger padding come from the score's configured staff weight?

ecstrema · 2022-04-11T18:06:29Z

src/engraving/libmscore/score.cpp

+     * but we introduce an appropriate exception for same-voice cases in Shape::minHorizontalDistance().
+     */
+    _paddingTable[ElementType::NOTE][ElementType::NOTE] = minimumPaddingUnit;
+    _paddingTable[ElementType::NOTE][ElementType::LEDGER_LINE] = 0.35 * spatium();


ecstrema · 2022-04-11T18:09:05Z

src/engraving/libmscore/score.cpp

+    _paddingTable[ElementType::STEM][ElementType::STEM] = 0.85 * spatium();
+    _paddingTable[ElementType::STEM][ElementType::ACCIDENTAL] = 0.35 * spatium();
+    _paddingTable[ElementType::STEM][ElementType::LEDGER_LINE] = 0.35 * spatium();
+    _paddingTable[ElementType::LEDGER_LINE][ElementType::STEM] = 0.35 * spatium();


All these magic numbers may need to be modified with other music fonts.

Did you just set these values because you tried them?

If yes, you tried them with Leland I guess?

The magic numbers emanate from me (@mike-spa working here under my brutal and unforgiving instruction).

In the end we want all the values of the padding table to be user-configurable. We can't do that right now as there's a high design cost and it will need a lot of thought to work out how to expose them all, since there are so many values and potential interactions between them.

For now we're taking the settings that do already exist (in the Style -> Measure/Bar dialog) and using them in the relevant slots in the table, and then deriving other values from them where possible. For the remainder I have supplied defaults for the time being, which aren't arbitrary.

Sounds good then. It can always be made configurable when users ask for it I guess.

ecstrema · 2022-04-11T18:11:02Z

src/engraving/libmscore/score.h

@@ -513,6 +513,9 @@ class Score : public QObject, public EngravingObject
    void assignIdIfNeed(Staff& staff) const;
    void assignIdIfNeed(Part& part) const;

+    PaddingTable _paddingTable;
+    void createPaddingTable();


is the padding table updated when the user changes score properties?

Jojo-Schmitz · 2022-04-13T08:31:51Z

"8 - engraving_utests (SEGFAULT)"

mike-spa · 2022-04-13T08:49:08Z

"8 - engraving_utests (SEGFAULT)"

Yeah I saw that. I never had that kind failure: how can I replicate that specific test on my machine to debug it?

Jojo-Schmitz · 2022-04-13T09:15:16Z

Not sure at all. There are other failures too, diffs. Maybe those in the end result in a crash?

mike-spa · 2022-04-13T09:19:52Z

Yeah I'm now going through the diffs and fix those, then I'll force push again and let's see what happens

Complete overhaul of horizontal spacing collision management. Items are finally allowed to kern above/below each other.

and changed in all the templates

mike-spa · 2022-04-14T12:49:18Z

@igorkorsukov I got a compiler error after the last rebase related to a QList vs std conflict. I hope I've fixed it correctly

codestyle

Eism · 2022-04-21T15:45:10Z

src/engraving/rw/scorereader.cpp

+    // These are necessary to be sure that ALL the layout is correctly updated when loading any file
+    masterScore->styleChanged();
+    masterScore->update();
+


@mike-spa can we remove this block of code?
This code takes a lot of time when opening a big score.
You can test on the score from this PR #8143

This code does not solve all the problems with the long opening, but it significantly reduces the time.
And the comment says that this code may not be executed, we just make sure.

@Eism I had to do that because many scores were not loading the new spacing settings correctly, so for now we can't just remove the code, unfortunately. But there is probably a smarter way to do it, I can try to look into that. Just for my understanding: which one of them is more time consuming? styleChanged()or update()?

Side note: #8143 was opened before this PR was merged, so there must be also something else going on, I guess.

Score::update

Excellent, thank you. I'll try to look into it as soon as I can!

mike-spa force-pushed the elementsCanTuck branch from dce2f17 to 2bc67f4 Compare April 5, 2022 16:19

ecstrema reviewed Apr 6, 2022

View reviewed changes

mike-spa force-pushed the elementsCanTuck branch 6 times, most recently from 13b7758 to 16847fc Compare April 11, 2022 17:26

ecstrema reviewed Apr 11, 2022

View reviewed changes

mike-spa force-pushed the elementsCanTuck branch 2 times, most recently from b2d48c3 to 4f505a8 Compare April 12, 2022 14:01

oktophonie approved these changes Apr 12, 2022

View reviewed changes

oktophonie added the vtests This PR produces approved changes to vtest results label Apr 12, 2022

mike-spa force-pushed the elementsCanTuck branch from 4f505a8 to 43f14c5 Compare April 13, 2022 07:56

mike-spa force-pushed the elementsCanTuck branch from 772e62e to 9c351ab Compare April 13, 2022 12:33

mike-spa force-pushed the elementsCanTuck branch from 9c351ab to f955849 Compare April 13, 2022 13:07

mike-spa added 4 commits April 14, 2022 14:36

Collisions overhaul

b22b519

Complete overhaul of horizontal spacing collision management. Items are finally allowed to kern above/below each other.

default minNoteDistance change

c488a49

and changed in all the templates

added new v-tests

44aae0a

utest fixes

137e8d4

mike-spa force-pushed the elementsCanTuck branch from f955849 to a0e69a5 Compare April 14, 2022 12:48

replaced QList to std::vector

bc0455d

codestyle

mike-spa force-pushed the elementsCanTuck branch from a0e69a5 to bc0455d Compare April 14, 2022 12:54

vpereverzev merged commit ff75767 into musescore:master Apr 14, 2022

Eism reviewed Apr 21, 2022

View reviewed changes

mike-spa mentioned this pull request Apr 26, 2022

Update spacing settings when loading up new scores #11397

Merged

RomanPudashkin mentioned this pull request Apr 30, 2022

[MU4 Issue] Unable to Save Text Styling #11375

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Collisions overhaul (and other improvements to horizontal spacing) #11099

Collisions overhaul (and other improvements to horizontal spacing) #11099

mike-spa commented Apr 5, 2022

MarcSabatella commented Apr 6, 2022

ecstrema Apr 6, 2022

mike-spa Apr 6, 2022

ecstrema Apr 6, 2022

ecstrema commented Apr 6, 2022

ecstrema commented Apr 6, 2022

mike-spa commented Apr 6, 2022

mike-spa commented Apr 6, 2022

ecstrema Apr 11, 2022

ecstrema Apr 11, 2022

oktophonie Apr 12, 2022

ecstrema Apr 11, 2022

ecstrema Apr 11, 2022

ecstrema Apr 11, 2022

oktophonie Apr 12, 2022

ecstrema Apr 12, 2022

ecstrema Apr 11, 2022

Jojo-Schmitz commented Apr 13, 2022

mike-spa commented Apr 13, 2022

Jojo-Schmitz commented Apr 13, 2022

mike-spa commented Apr 13, 2022

mike-spa commented Apr 14, 2022

Eism Apr 21, 2022

mike-spa Apr 22, 2022

Eism Apr 22, 2022

mike-spa Apr 22, 2022

Collisions overhaul (and other improvements to horizontal spacing) #11099

Collisions overhaul (and other improvements to horizontal spacing) #11099

Conversation

mike-spa commented Apr 5, 2022

MarcSabatella commented Apr 6, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ecstrema commented Apr 6, 2022

ecstrema commented Apr 6, 2022

mike-spa commented Apr 6, 2022

mike-spa commented Apr 6, 2022

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

Jojo-Schmitz commented Apr 13, 2022

mike-spa commented Apr 13, 2022

Jojo-Schmitz commented Apr 13, 2022

mike-spa commented Apr 13, 2022

mike-spa commented Apr 14, 2022

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