Fix #299741 and #289446 - Improved horizontal spacing algorithm #9928
Conversation
|
The vtests failures are probably to be expected, but the failures on codestyle (pretty straight forward to fix) and utest (why failing on beams???) certainly are not. Seems a |
|
Wow, thanks for tackling this! A surprisingly small amount of code for what it is trying to accomplish. It would be great to see some samples of it in action; some new vtests would be a good idea assuming we're still doing that. |
Forgive me guys, I'm a total noob. This is the first time I've ever contributed to an open source project so I'm a bit overwhelmed. I don't know what these tests mean and why the code is failing them, or how I should fix that. What should I do? |
|
First check on the code style check, it does tell you exactly how it wants the code to be formatted like to pass the test. Low hanging fruit ;-) Why the utests fail is beyond me. Not sure whether they do reveal a coding problem or it is just the tests that need to get adjusted. The vtests are, as mentioned, supposed to fail, and now with that |
|
You've done a great job, don't worry about the weird github particulars. If you look at the 'Details' link next to the failing CI_codestyle check, you can see where it wants you to make changes. From looking at it myself, it seems like the majority of the cases are just adding brackets to conditionals and putting brackets for functions on the next line. Once you fix those, you can force push your changes to the same branch and it'll run the checks again. |
|
Welcome @mike-spa! This is a great first commit. To answer a few of your questions about the tests: utestsThe "utests" are our unit tests, and they help ensure things work properly. One of the things it checks for are if the beams are at the correct vertical positions. If you click "details" next to the failing tests, you'll see some snippets that look like this: This essentially tells you the difference between the previous xml output and the new xml output. The Now why is it different when you didn't touch the beam code? A beam's slant is partially determined by the spacing between the first and last notes of the beam group. So when you adjust the horizontal spacing, I'd expect some beams to have different slants (and therefore change their vertical positions). I need to investigate further to know if this is the exact reason, but it's expected that these tests would fail. You probably don't need to worry about the utests failing right now. vtestsVtests are just visual snapshot tests. Imagine exporting the score as a PNG from both the current version of MuseScore and the version with your proposed changes. Then, compare those PNGs pixel by pixel to see if there's a difference. That's essentially how the vtests work. So anytime you make visual changes to the engraved layout, it's possible the vtests will fail. The next question to ask is "why did the vtests fail?" If the changes are the exact visual changes you're expecting, great! No need to worry about fixing them (the change is desired). If not, well, that's what the vtests are for. I looked through the test output, and everything looks good. The only differences deal with horizontal spacing, and it looks like it's the kind of changes you intentionally made. I wouldn't worry about fixing them. Hope this helps! |
Aha! That makes sense, thanks a lot! The vtest is supposed to fail, and you're probably right about the beams, that's a relief! I've re-committed the code correcting for the style errors, it should be ok now! |
|
OK, so it seems (!) we'd then need to adjust those utest to the changed spacing. @Nick-Mazuk are you investigating into that? |
Hoping to get to that later today. If not, tomorrow. |
|
BTW, in Discord I mentioned my trial PR #6312 as something also worth revisiting in this context (like seeing how well it still applies to MuseScore 4; my code was only for 3). It's a pretty simple change and in a different part of the code from this PR, but I think they kind of go hand in hand in terms of both addressing issues with inconsistent spacing. This PR is about spacing caused by bad decisions about stretch; my PR is about allocating too much space between notes in the first place, in cases where notes overlap between staves. Another relevant PR to think about is #5840. This is about accidentals not tucking under/over earlier notes as was actually originally implemented by Werner for MuseScore 3 many years ago but was backed out before release. As far as I can tell, the only issue preventing it from working was that we weren't sure about stem direction until too late, because beaming code didn't decide on stem directions until later on. I have no idea if the new beaming code changes this, but anyhow, this seemed as good a place as any to point out that there is some existing work that could possibly be leveraged and to make sure the people working on engraving improvements know about it. I am not in a position to take this back up right now myself. |
|
Still code style issues, 2 are about trailing space, 2 others about the bracing of an |
|
So I've gone through the unit tests in more detail, and it was definitely the beams causing the tests to fail. Also, my theory was correct and the different spacing caused the vertical positions of the beams to change. Finally, I can confirm that all the beams look good. @mike-spa, you'll need to update the test files to make sure all the tests pass. Unfortunately, I cannot update the PR to do this for you. Here's how you can do this. First, you'll need to look through the test logs to find several sections that look like this. This will tell you exactly what you need to update. There are three things to note:
So for this case, you'll just need to replace the In total, there are 8 files that need to be changed:
|
|
@Nick-Mazuk: You could though create a PR on top of @mike-spa's branch for this PR, which he'd then just need to merge. BTW: These unit tests seem to be the reversed, diff new old rather than diff old new, resulting in a somewhat confusing output. |
|
@Nick-Mazuk |
Would you mind creating an issue about that with more detail and potentially an example or two? If stems are randomly flipping that is likely a bug. There is one exception: notes on the middle line (or chords that are symmetrical around the middle line) will flip their stem direction based on the context. |
|
Aha! Yes, the flips I experienced were all about the middle line. Maybe it's just that I don't understand the rule behind the flips, but they did feel quite random. I'll crete the issue later, and if the behaviour is as intended I'll remove it. Meanwhile, do you still want me to modify the utest file? Because my concern was: if I change the utest file to pass the test, but the test failure was caused by a possible bug, then I'm basically adapting the test to the bug, which wouldn't be good, right? |
Yes, go ahead and fix them. That flipping shouldn't affect any of the tests. Saw your issue #9959, and yeah, the behavior is buggy and confusing. So I created PR #9969 to disable that feature. Not sure if we're going to fix it or scrap it altogether (probably the latter). When creating this PR, though, I didn't have to modify any of the tests, so the utests here shouldn't be failing because of the context flipping. Anyways, that flipping was not based on the horizontal spacing whatsoever. |
|
I believe the unit tests still need to get adjusted as per the above. Also you probably should rebase, as the vtest mechanism has been changed by quite a bit very recently. |
|
That merge is the wrong way of doing things with PRs on MuseScore. Instead try |
src/engraving/libmscore/measure.cpp
Outdated
| @@ -3160,6 +3160,10 @@ void Measure::stretchMeasure(qreal targetWidth) | |||
|
|
|||
| std::multimap<qreal, Segment*> springs; | |||
|
|
|||
| //Fraction minTicks = computeTicks(); | |||
| Fraction minTicks = minSysTicks(); // The stretching must be done according to the shortest note *of the system*! | |||
| qreal minNoteSpace = (score()->noteHeadWidth() + score()->styleMM(Sid::minNoteDistance)); | |||
There was a problem hiding this comment.
This one is not needed, not used anywhere, leads to a compiler warning (in MSCV)
| @@ -381,8 +422,8 @@ System* LayoutSystem::collectSystem(const LayoutOptions& options, LayoutContext& | |||
| mb->setPos(pos); | |||
| Measure* m = toMeasure(mb); | |||
| qreal stretch = m->basicStretch(); | |||
There was a problem hiding this comment.
This one is now no longer needed (nor used) and leads to a compiler warning (in MSVC)
|
See Jojo-Schmitz@b7b0fac for a commit fixing all these issue (that one remaining code style issue, the 3 compiler warnings and those 8 unit tests) |
mike-spa
force-pushed
the
spacing_improvements
branch
from
43a57f8
to
095fbac
Compare
|
Almost there, one last unit test failing, for ...src/engraving/utests/beam_data/Beam-E.mscx: Probably my fault from Jojo-Schmitz@b7b0fac |
|
P.S. Dare you to make a 3.x compatible version for self-compiling folks in-between releases ;-) |
|
That wouldn't be a bad idea ;) I may do that if I happen to find the time. |
|
It'd be much appreciated. Doesn't seem to be too straight forward though. |
mike-spa
force-pushed
the
spacing_improvements
branch
from
eccdd74
to
c2227cb
Compare
| qreal stretchCoeff = 1; | ||
| qreal prevWidth = 0; | ||
| int iter = 0; | ||
| while (abs(newRest) > score->spatium() * 0.08 && iter < 200) { // spatium*0.08 is about the width of a note stem |
There was a problem hiding this comment.
looks like if I'll change a width of a stem in "Properties" panel or in style we'll not layout this properly?
makes sense to use the value from the current style or make a constant variable at stem.cpp
There was a problem hiding this comment.
No, sorry, my comment there was misleading. This layout is totally independent of the stem width, I was just using the stem width as a comparison, to give a generic idea of "how small" is 0.08 sp. I'll change this into something more clear!
There was a problem hiding this comment.
You could do something like
// For reference: this is approximately as small as the width of a normal stem of a note
// (or a similar comment, if we think a comparison like this is needed at all.)
qreal epsilon = score->spatium() * 0.08;
while (abs(newRest) > epsilon && iter < 200) {
// ...This also eliminates recalculating score->spatium() * 0.08 again and again, which might be a few nanoseconds faster :)
There was a problem hiding this comment.
Good one, thanks! :)
|
FWIW, we use that same trick of choosing an arbitrary large default value a number of other places n the code as well, and my recollection is we usually use "10000" (ten thousand). At least, for other values in a similar basic range, like for MIDI pitch numbers of staff line numbers or offsets in sp units. |
@mike-spa |
The compiler doesn't let me do that. I guess it's because I need to do include Fraction.h into constants.h if I want to declare a Fraction, but Fraction.h also includes constant.h at its beginning. Or am I missing something? |
you can also add this variable in fraction.h I think class Fraction { |
mike-spa
force-pushed
the
spacing_improvements
branch
from
22d62e1
to
99e637c
Compare
@vpereverzev thank you, I'm gonna try that tomorrow. For now, I've pushed some new code with all the other fixes you suggested. |
src/engraving/libmscore/system.cpp
Outdated
| @@ -1832,14 +1832,13 @@ int System::lastVisibleSysStaffOfPart(const Part* part) const | |||
|
|
|||
| Fraction System::minSysTicks() const | |||
| { | |||
| Fraction minTicks = Fraction(10, 1); // Initializing the variable at a random high value. | |||
| Fraction minTicks = Fraction (10000, 1); // Initializing the variable at a random high value. | |||
There was a problem hiding this comment.
There's a space too much for the code style check
mike-spa
force-pushed
the
spacing_improvements
branch
from
99e637c
to
0a61d46
Compare
(I hope I'll get it right at some point)
By passing the shortest note of the system to computeMinWidth() (instead of computing it *inside* the method itself) we save thousands of unnecessary loops through the measures (especially when in continuos view). This required to change the definition of the method and each time it was called. Side benefit: also improved the behaviour of spacing algorithm in continuos view.
The 'minimum note distance' parameter now only affects notes at- or close to the minimum. Implemented a more refined way to increase the default spacing of longer notes, to make sure they're not too narrow in the absence of shorter ones.
Grabbed from Jojo (thanks!)
Implemented dynamic spacing slope. Smarter decisions on how many measures to fit on a system. Rewritten the whole measure stretch algorithm. Finally fixed the problem of bars with many accidentals being too wide.
Renamed a couple of methods to better reflect what they do now. Deleted some unnecessary code.
Will be moved to a separate PR with other style changes by @oktophonie
mike-spa
force-pushed
the
spacing_improvements
branch
from
0a61d46
to
735421f
Compare
|
Fantastic! Congratulations on the great work, @mike-spa! |
Fix #299741 and #289446
Ensures consistent spacing of same-duration notes within the same system.
Behaves much better with measures containing many accidentals.
Has customizable spacing formula (possibly to be chosen in Style settings).
The default spacing formula now perfectly replicates the spacing suggested by Gould.
Provides smoother behaviour to the } and { stretch commands.
Please see attached PDF for full documentation.
An improved horizontal spacing algorithm for Musescore.pdf