Add method to convert piecewise linear curves to spline control points #6056

OliBomby · 2023-11-20T22:57:29Z

Summary

This PR introduces an algorithm for approximating arbitrary piecewise linear paths with B-splines or Bezier curves. The method PiecewiseLinearToBSpline is then used to improve the paths generated by IncrementalBSplineBuilder, making the curve accurately follow the input path using as few control points as possible.

Adds methods PiecewiseLinearToBezier and PiecewiseLinearToBSpline to PathApproximator.
Adds TestScenePiecewiseLinearToBSpline to interactively test the algorithm on various curves with controllable parameters.
Adds BenchmarkPiecewiseLinearToBezier to benchmark the performance of the algorithm.

Utility

This addresses two points of concern mentioned in ppy/osu#25409:

It's very hard to use the freehand to create a shape you're trying to make.

The generated path more closely follows your input, so it should be easier to create the shape you are actually trying to create because you dont have to manually account for the inward bias.

The resultant points are too frequent to provide a good manual tweaking experience after creating

The path requires less control points to still follow your input, so there are less control points to require tweaking afterwards.

TODOs

A later PR will introduce interop with IncrementalBSplineBuilder. I plan to improve performance by re-using results from previous frames so fewer iterations are necessary per frame to optimize the control points (this also would decrease wobbling).

Also weight matrices can be cached in between frames for even more performance gains. I also want to use the total winding to increase precision in the approximation algorithm when necessary (on very long, complex paths).

Usability can be improved by locking previous control points which have been sufficiently optimised, so there is no wobble in the entire path while drawing the path.

Benchmarks

I added a nuget package to handle efficient large matrix multiplication, for this I added Tensor.NET. I chose this with performance in mind. I added a benchmark and compared performance against NumSharp, where Tensor.NET won by a large margin. Then I rewrote it to pure C# because Tensor.NET is not compatible and it got way more performant. Super large matrices might still be faster in Tensor.NET but that wont be the case most of the time.

Benchmark using NumSharp:

Benchmark using Tensor.NET (about 10x faster and comparable to native numpy in python):

Benchmark using pure C# and optimized allocations:

Benchmark using System.Numerics.Tensors for SIMD:

@Tom94 Please give feedback, what do you think about the results?

osu.Framework.Tests_I2au6DMQHJ.mp4

…ion-tensor

bdach · 2023-11-20T23:50:43Z

There's this and there's #6055. I'm not even sure what to do with these right now.

@OliBomby At the point where this sort of stuff is happening, I'd appreciate at least a minimum effort of communication before opening this.

osu.Framework/osu.Framework.csproj

OliBomby · 2023-11-21T00:42:29Z

There's this and there's #6055. I'm not even sure what to do with these right now.

@OliBomby At the point where this sort of stuff is happening, I'd appreciate at least a minimum effort of communication before opening this.

I had mentioned in #6044 (comment) that I'd open this PR. It's mostly meant for @Tom94 to compare the utility this algorithm provides.

You may ignore reviewing code for now because I'll probably have to rewrite most of it anyways.

Tom94 · 2023-11-21T06:00:35Z

Hi, I find the path approximations from the video very impressive for the relatively small amount of code used in the reconstruction! Thanks a bunch for the follow-up.

I'd say this PR is orthogonal to the existing BSpline builder. We should probably keep the existing hard corner detection while being able to either drop-in replace the winding-based control point placement in between corners or use it as initialization for this algorithm. I feel like it'd be valuable to have this in framework in any case as something like a self-contained List<Vector2> optimizeBSpline(List<Vector2> initialCps, List<Vector2> targetPath) function that can be optionally enabled in IncrementalBSplineBuilder once it becomes production ready (perf and otherwise).

As you already point out, there are a few rough edges, but I think they can be ironed out. My main gripes would be:

Control points are very far from the curve at high Tolerance values... but I think that regime is not so important anyway. A lower default value, combined with a (potentially tweakable) hard constraint on the maximum distance from the curve could solve this.
The entire curve wobbles a lot when new points are added -- but you seem to already be on top of this in your TODO list. :)
For the test scene, I recommend adding lines that visually connect the control points so it's easier to see which part of the BSpline they correspond to when there are self-intersections. This wasn't needed when control points would always fall on the input path, but now I find myself confused with some of the reconstructions.

peppy · 2023-11-21T08:00:04Z

Control points are very far from the curve at high Tolerance values

Not looking too deep into this, but just want to add that this is a blocker in terms of osu! side usability. Control points generated by all settings exposed to the user must look sane to the user so they can easily edit/tweak the curve after drawing it.

OliBomby · 2023-11-21T20:30:08Z

Good news! I've rewritten the algorithm to not use Tensor.NET and be pure C# instead, and its actually 2x-10x faster and uses 100x times less heap allocations. I didn't expect a naive implementation without SIMD to still be this fast. Benchmark below.

Though there is a lot more code now for all the matrix operations, so it might warrant being put in its own class.

Not looking too deep into this, but just want to add that this is a blocker in terms of osu! side usability. Control points generated by all settings exposed to the user must look sane to the user so they can easily edit/tweak the curve after drawing it.

The distance between control points and the curve depends on the locality and number of control points. I think if we restrict the order to be sufficiently low and add enough control points, the result should be close enough to the curve to look sane.

For the test scene, I recommend adding lines that visually connect the control points so it's easier to see which part of the BSpline they correspond to when there are self-intersections. This wasn't needed when control points would always fall on the input path, but now I find myself confused with some of the reconstructions.

I think this is a good addition, it lets us see more accurately if the control points are 'sane'.

remove tensor.net and implement with pure c#

osu.Framework/Utils/PathApproximator.cs

This reverts commit c7745ae.

This reverts commit 6724305.

This reverts commit c112ed0.

This reverts commit f847b3f.

OliBomby · 2023-11-24T15:29:46Z

I've removed the changes to IncrementalBSplineBuilder to save that for another PR and not make this PR too huge. I've now marked this ready for review just for the PiecewiseLinearToBezier and PiecewiseLinearToBSpline methods.

bdach

code is underdocumented and opaque

osu.Framework/Utils/PathApproximator.cs

bdach · 2023-12-02T19:55:38Z

osu.Framework/Utils/PathApproximator.cs

+        /// <returns>Matrix array of B-spline basis function values.</returns>
+        private static float[,] generateBSplineWeights(int numControlPoints, int numTestPoints, int degree)
+        {
+            // Calculate the basis function values using a modified De Boor's algorithm


Modified how and why? Is there a citation for this?

(I'm not even attempting to check the correctness of this implementation until this review is responded to.)

I guess this comment is not accurate. I didn't use any specific algorithm as reference.
I started with the Cox-de Boor recursion formula and turned it into my own algorithm by applying some optimizations. First I used dynamic programming to remove the recursion, and then I realized some steps can be ignored because the knots on the edges are all zero width.
What I ended up with looked pretty similar to De Boor's algorithm so I added that comment to point to some resources which might explain what's going on here.
I should change the comment to say that this method is just using Cox-de Boor recursion formula, but highly optimized.

osu.Framework/Utils/PathApproximator.cs

peppy · 2023-12-07T02:45:15Z

I'd personally like to get this in with minimal review overhead (along with #6066 / ppy/osu#25658).

bdach

still think this is opaque in places but since i'm getting told to move this on -

bdach · 2023-12-07T08:22:00Z

osu.Framework/Utils/PathApproximator.cs

+
+            for (int i = 0; i < numTestPoints; i++)
+            {
+                prevOrder[i, (int)MathHelper.Clamp(x[i] * (numControlPoints - degree), 0, numControlPoints - degree - 1)] = 1;


I'm not sure why the upper range of the clamp is numControlPoints - degree - 1, but changing it to what I would consider more correct breaks things, so there must be a reason for this that I'm not understanding...

The reason why I was suspicious is that while the left-side clamp looks effectively dead (because x[i] should never be less than 0), the right-side clamp looks like it may cut off values on the right side. (the last one, specifically; x[^1] == 1, and as such, numControlPoints - degree will get clamped).

In the Cox-de Boor recursion formula, the first order is basically sorting this x[i] into a knot whose range includes x[i]. Because this is an open uniform knot vector, we got degree number of zero-width knots on the end-points and numControlPoints - degree number of equally spaced knots in the middle. The knots range is usually not inclusive on the right side but in this case i want the last non zero-width knot to be inclusive on the right side, so the non-zero width knots include the whole range [0, 1]. This is exactly what the clamp achieves here.

bdach · 2023-12-07T08:33:33Z

osu.Framework/Utils/PathApproximator.cs

+                    // This code multiplies the previous order by equal length arrays of alphas and betas,
+                    // then shifts the alpha array by one index, and adds the results, resulting in one extra length.
+                    // nextOrder = (prevOrder * alphas).shiftRight() + (prevOrder * betas)
+                    float prevAlpha = 0;
+
+                    for (int j = 0; j < numControlPoints - degree + q - 1; j++)
+                    {
+                        float alpha = (x[i] - knots[degree - q + 1 + j]) / (knots[degree + 1 + j] - knots[degree - q + 1 + j]);
+                        float alphaVal = alpha * prevOrder[i, j];
+                        float betaVal = (1 - alpha) * prevOrder[i, j];
+                        prevOrder[i, j] = prevAlpha + betaVal;
+                        prevAlpha = alphaVal;
+                    }
+
+                    prevOrder[i, numControlPoints - degree + q - 1] = prevAlpha;


Zero citations, zero anything. I basically don't know where any of this comes from, but since I'm told to keep review minimal, I suppose I will just look away.

bdach · 2023-12-07T08:41:01Z

osu.Framework/Utils/PathApproximator.cs

+                matmul(controlPoints, weights, points);
+
+                // Update labels to shift the distance distribution between points
+                if (step % 11 == 0)


random "every eleventh step" (why eleventh?)

bdach · 2023-12-07T08:47:39Z

osu.Framework/Utils/PathApproximator.cs

+                matLerp(grad, m, b1, m);
+                matProduct(grad, grad, grad);
+                matLerp(grad, v, b2, v);


The fact that matLerp() mixes the first operand with weight 1 - t and the second with weight t makes this so confusing to read....... When cross-referencing against the adam paper I almost thought this had it backwards for a second.

OliBomby added 15 commits November 17, 2023 19:18

implement bezier approximation algorithm

52390d5

fixes and added test

e2c0d92

enable optimization process later to improve loading time

4133d6e

clean up and rename stuff

54e96a8

add benchmark

f8f6a4c

numpy.net implementation

b621984

Tensor.NET implementation

d9358d1

add optimized B-Spline basis function matrix

adf7c83

make method private

5d9b789

Added B-Spline optimization and test

e4172c0

add initial control points arg

345b81a

Add naive use of optimizer to IncrementalBSplineBuilder

f847b3f

Merge remote-tracking branch 'upstream/master' into bezier-approximat…

b657ac1

…ion-tensor

use smooth path for shape

c112ed0

increase max iterations

6724305

pull-request-size bot added the size/XL label Nov 20, 2023

bdach reviewed Nov 21, 2023

View reviewed changes

osu.Framework/osu.Framework.csproj Outdated Show resolved Hide resolved

remove tensor.net and implement with pure c#

27e5a9d

OliBomby added 7 commits November 21, 2023 21:36

Merge pull request #1 from OliBomby/bezier-approximation-pure

cfa1a77

remove tensor.net and implement with pure c#

code quality fixes

c7745ae

fix properties in test

0ef8309

SIMD optimized matmul

fddf1be

remove matmulTranspose

67db61b

SIMD on other matrix ops

4ce570e

update comments

69f9799

simplify vectorizations

1399a1e

smoogipoo reviewed Nov 22, 2023

View reviewed changes

osu.Framework/Utils/PathApproximator.cs Outdated Show resolved Hide resolved

OliBomby added 5 commits November 24, 2023 10:20

Add useful comments

2782512

Revert "code quality fixes"

fe9eb50

This reverts commit c7745ae.

Revert "increase max iterations"

8ab1d6c

This reverts commit 6724305.

Revert "use smooth path for shape"

30daa08

This reverts commit c112ed0.

Revert "Add naive use of optimizer to IncrementalBSplineBuilder"

9368494

This reverts commit f847b3f.

OliBomby marked this pull request as ready for review November 24, 2023 15:22

OliBomby changed the title ~~Optimize anchor placement for incremental B-spline builder~~ Add method to convert piecewise linear curves to spline control points Nov 24, 2023

OliBomby mentioned this pull request Nov 29, 2023

Improve incremental B-spline builder via stochastic optimisation #6066

Merged

1 task

bdach reviewed Dec 2, 2023

View reviewed changes

OliBomby added 7 commits December 3, 2023 14:47

remove interpolator resolution arg

506c2ae

add xmldoc and fix comment

5882173

add checks for weight generating func arguments

23dc4e7

move comment

5e075ad

Changed binomialCoefficients to return n+1 terms

30a924f

Add xmldoc to getDistanceDistribution

46c9471

Add argument check for matLerp

0f8d3cc

OliBomby mentioned this pull request Dec 3, 2023

Improve free-hand drawing of sliders to the editor ppy/osu#25658

Merged

2 tasks

bdach approved these changes Dec 7, 2023

View reviewed changes

Merge branch 'master' into bezier-approximation-tensor

a3f3fa6

bdach enabled auto-merge December 7, 2023 09:03

bdach merged commit 44f2f93 into ppy:master Dec 7, 2023
20 of 21 checks passed

OliBomby deleted the bezier-approximation-tensor branch December 13, 2023 10:57

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add method to convert piecewise linear curves to spline control points #6056

Add method to convert piecewise linear curves to spline control points #6056

OliBomby commented Nov 20, 2023 •

edited

Loading

bdach commented Nov 20, 2023

OliBomby commented Nov 21, 2023 •

edited

Loading

Tom94 commented Nov 21, 2023

peppy commented Nov 21, 2023 •

edited

Loading

OliBomby commented Nov 21, 2023

OliBomby commented Nov 24, 2023

bdach left a comment

bdach Dec 2, 2023

OliBomby Dec 3, 2023

peppy commented Dec 7, 2023

bdach left a comment

bdach Dec 7, 2023

OliBomby Dec 7, 2023

bdach Dec 7, 2023

bdach Dec 7, 2023

bdach Dec 7, 2023

Add method to convert piecewise linear curves to spline control points #6056

Add method to convert piecewise linear curves to spline control points #6056

Conversation

OliBomby commented Nov 20, 2023 • edited Loading

Summary

Utility

TODOs

Benchmarks

bdach commented Nov 20, 2023

OliBomby commented Nov 21, 2023 • edited Loading

Tom94 commented Nov 21, 2023

peppy commented Nov 21, 2023 • edited Loading

OliBomby commented Nov 21, 2023

OliBomby commented Nov 24, 2023

bdach left a comment

Choose a reason for hiding this comment

bdach Dec 2, 2023

Choose a reason for hiding this comment

OliBomby Dec 3, 2023

Choose a reason for hiding this comment

peppy commented Dec 7, 2023

bdach left a comment

Choose a reason for hiding this comment

bdach Dec 7, 2023

Choose a reason for hiding this comment

OliBomby Dec 7, 2023

Choose a reason for hiding this comment

bdach Dec 7, 2023

Choose a reason for hiding this comment

bdach Dec 7, 2023

Choose a reason for hiding this comment

bdach Dec 7, 2023

Choose a reason for hiding this comment

OliBomby commented Nov 20, 2023 •

edited

Loading

OliBomby commented Nov 21, 2023 •

edited

Loading

peppy commented Nov 21, 2023 •

edited

Loading