Fixing some quirky slur rendering

[RhinoHaggis]

I'd like to start getting familiar with the MuseScore codebase, but I'm not a real coder, and I was hoping I could get a little help. I've contrived a silly project and maybe someone can help direct me.

Here is a largely fictional bug report about slur rendering:

1. Looping a slur over itself exposes an unfortunate underlying rendering geometry
2. Bringing a slur baseline vertical makes the slur stop responding, and then vanish. Bad things can be done to the control points, and to no effect.

Now. This is actually true:

the party slur.mscz.zip

However, it's hard to imagine a usage case for either.

This is an excuse to dig into the codebase. I am not a coder, but I am a mathematician, and I know a fair amount about splines. I'd like to find where the slur shape and rendering are defined from the control points and see how you've implemented it. I generally expect rendering to be highly optimized, but there's always a chance I can clean up the behavior and make things a bit more efficient... and easily extensible to multi-segment splines, allowing more control over the curvature (especially for long slurs), and allow more complex slur shapes.

Can someone help me get oriented?

[RhinoHaggis]

I'm expecting the slur render to be deliberate: a simplification that is not exposed under any normal usage, and that avoids the extra calculation needed to (correctly) define the slur as a closed planar figure, and fill it as such. However, the slurs crapping out when the baseline goes vertical makes me suspect the spline is defined in an unfortunate and costly way, and that we might be able to fix the latter, with enough savings left over to correct the former... and still come out ahead.

[RhinoHaggis]

Ok well in the meantime…
what I see on the surface is an order 3 Bézier curve and its four defining points, the bounding box (unrestricted quadrilateral) of the slur, with added midpoints on the baseline and upper line, to shift the whole slur and the general upper shape, respectively, but having no contribution to the defining equation.

[RhinoHaggis]

So then there is the question of how you’ve defined the width.
-Upper curve and lower curve, and a method of filling the space between?
-A single spline down the center, with a fattening parameter along its Normal?

The behavior of the slur on the right (above) seems to rule out the latter. I have a guess how the shading is being done, based on this curious behavior, so i’ll go see if I can reproduce it from scratch.

If someone can direct me to these parts of the code I’d be obliged 🙏

[cbjeukendrup]

@RhinoHaggis Sorry that I couldn't reply sooner, but here are some pointers:

• The basic bezier calculation takes place in SlurTieLayout::computeBezier:
  • For slurs:

    MuseScore/src/engraving/rendering/score/slurtielayout.cpp

    Line 2136 in ace8209

    void SlurTieLayout::computeBezier(SlurSegment* slurSeg, PointF shoulderOffset)

  • For ties:

    MuseScore/src/engraving/rendering/score/slurtielayout.cpp

    Line 2054 in ace8209

    void SlurTieLayout::computeBezier(TieSegment* tieSeg, PointF shoulderOffset)

• But there is a whole lot of logic around it: check where computeBezier is called, or, more generally, where bezier occurs in slurtielayout.cpp
• Drawing happens in TDraw::draw(const SlurSegment* item, Painter* painter), but that's quite boring, because that just draws the path calculated during the layout phase

To my regret, I'm personally not very familiar with the details, but I hope that this gets you started!
And, @mike-spa knows a lot about this area!

[RhinoHaggis]

thank you!!

[RhinoHaggis]

Ok my first question is are you committed to all of these atan() and Rotate() calls? All rotations can be performed by direct transformation of the x, y components, with a simple component wise (parallel) MADD, without ever calculating the angle. This will also bulletproof you against any unfortunate behavior if the user moves the control points in a way you don’t expect (atan() can’t tell left from right).

[mike-spa]

@RhinoHaggis the slur is defined by a Bezier curve. To be more precise, an outer bezier curve, and an inner bezier curve. The two curves share the same start and end points, but split in the middle as the inner curve has a lower shoulder height. The painter then fills the region between the two curves and that gives us the tapered shape (thick in the middle, thin at the endpoints). This isn't perfect, and it certainly won't work for arbitrary shapes if you go crazy with the shoulder points, but it's also very simple and efficient and it works for all of our reasonable use cases, so I don't think it's worth overengineering that aspect.

You are almost certainly correct about the arc tangent though, which obviously breaks down at 90 degrees. It's used in a bunch of different places around slurs. If you want to try improving the math there, you're welcome to! Just be sure that the end result is the same (when you'll make a PR, the vtests will tell us). Also, I have some big changes in the pipeline to improve the slur anti-collision algorithm (SlurTieLayout::avoidCollisions), so maybe don't touch too much the code around there or we'll step on each other :)

Also, please log an actual issue about that disappearing slur.

[RhinoHaggis]

Ok thanks!
I’ll make a working demo as well so you can see what I’m doing. Basically i’ll use sin(a+b) and cos(a+b) identities to rotate, which is what rotate() should be doing anyway, but doing it directly saves two steps:

1. conversion out of straight line geometry (sines and cosines) to arc length (radians)
2. conversion from arc length back to the sines and cosines we started with

This may be nonsense but I wonder if the biggest cost here isn’t the actual calculations. These are transcendental functions so there’s no way for the compiler or program to “see” that the conversion between reference and rotated positions is immediate: the code says it requires intervention of an interpreter.

I won’t alter anything about the reference or rotated positions themselves.

It’s not going to go quickly, I still have to get Git installed and figure out how to code properly 🤣.

The only way i see to “break” the upper-lower shading is in the near neighborhood of a perfect spike which I agree doesn’t seem like a real usage case. (Edit: nevermind, it's easy to create weirdness in other cases).

I’ll look for a good way to implement region-by-common-Normal, but it will take me time to explore the painter and figure out how to compare efficiency of different methods. Edit: a simple fill between the two curves should suffice, and be much simpler than the current solution (see below)

[RhinoHaggis]

Is a negative or zero span distance allowed? (If the angles were all correctly calculated, would a slur draw to the left?)

[RhinoHaggis]

I think the render-fill fix is simpler than we're imagining. Right now, when the bounding box flips two lines it's not expecting, incorrect and complex regions are shaded. This is a much more complicated task than filling between the two curves, which are already parameterized along their length. The boundary is predefined. Perhaps someone can correct me, but I see no way the current fill could be accomplished without the painter finding all intersections of the curves and breaking the space up into mutually exclusive regions.

Current behavior:

Simulated 'dumb' fill: The region from Curve1 to Curve2 shown by filling along the length:

[cbjeukendrup]

This part of the Qt documentation might be relevant: https://doc.qt.io/qt-6/qt.html#FillRule-enum
(since our "Painter" is basically a wrapper around QPainter)

[RhinoHaggis]

ok thanks

[RhinoHaggis]

wow that's frustrating. At first glance there's no option for "let me tell you what direction is inside and outside so you don't have to reverse engineer the fill, just draw it"

[RhinoHaggis]

ok it's supported just have to go about it the right way. More things to learn. SO MUCH TO LEARN

[RhinoHaggis]

I'm not planning on touching it, but what is the purpose of the reference orientation slur? There's no need to do this for the spline itself.. .is there a coding or UI reason? It seems to me that if there is one thing that distinguishes Musescore from any other Bezier objects I've used, it's the way the slurs jump about when the app is unhappy with the placement. Strange behavior measuring from offsets. If this invisible "wish you were here" position is the reference frame, it seems like it makes editing a real chore in addition to eating up a ton of code space. Is there some reason the curves can't just live in whatever position they're set?

[RhinoHaggis]

Just to be clear, the problem with atan() is not at 90 degrees. As long as +- inf register, it will correctly return +-pi/2. What it can’t do, is detect or return a left-facing angle. So while you might not think the user should make a left-facing slur, at present there is no choice: once pp2 crosses to the left of pp1 the whole implementation is broken by these angle calls, which is a shame because left alone in their natural representation bezier curvez are virtually unbreakable. Certainly the test for broken slur in the code is not a sufficiently strict condition to break a Bézier curve