Fix game running out of memory when approximating almost-straight perfect slider curves

[Hecatia-Lapislazuli]

A crash to desktop would occur in SliderPath.calculatePath caused by when the control points are all approximately aligned in one of the axis, and as the type is not linear, it tries to generate a path that is nigh on infinite (or so I suspect) in SliderPath.calculateSubPath.

Can be demonstrated in the editor when scaling a slider that previously had a curve, that has become flat via the handles.

A test has been added to demonstrate this. (Although maybe it can be optimized? I'm not happy using the Timeout attribute, but am unsure of any other way to check for the exception occurring)

The solution I've chosen is to treat nearly straight segments as linear, rather than their previous path type.

This check is based on (roughly) the distance each vector in the segment is away from the line formed by the first point with the farthest point. (Is technically slightly larger than this as it is based more so on the chord of a circle, but difference should be negligible)

Some odd behaviour (after it becomes flat, you can use the handles twice before scaling is locked in that axis; when flat, the slider can behave somewhat erratically when scaling vertically) does seem to occur with using the handles after, but I don't believe this is to do with the changes I made, rather just weirdness elsewhere.

[bdach]

What is the actual crash? What is the stack trace when it happens?

[Hecatia-Lapislazuli]

Ah, right. Forgot to add that.

Exception has occurred: CLR/System.OutOfMemoryException
An exception of type 'System.OutOfMemoryException' occurred in osu.Framework.dll but was not handled in user code: 'Array dimensions exceeded supported range.'

stacktrace.txt

[bdach]

If this is to prevent an "out of memory" condition then please remove the test or adjust it to test something else. Any test attempting to exercise oom is inherently unreliable.

At best the test should be checking that the conversion to linear logic is operating correctly, not attempting to reproduce oom crashes.

[bdach]

Also see other test failures.

Also see this logic (why isn't it kicking in? there should be no need to implement this sort of thing twice)

osu/osu.Game/Rulesets/Objects/Legacy/ConvertHitObjectParser.cs

Lines 354 to 358 in c7a89c3

	else if (isLinear(points[0], points[1], endPoint ?? points[2]))
	{
	// osu-stable special-cased colinear perfect curves to a linear path
	type = PathType.LINEAR;
	}

[Hecatia-Lapislazuli]

Am addressing other issues - my IDE didn't pick up some code quality things, and also reworking test.

As for why the logic doesn't work, I'd assume is because it's for newly placed HitObjects, not parsed ones. Would have to check.

[bdach]

and also reworking test

It's not even only the added test that is the problem, other conversion tests are failing. See https://github.com/ppy/osu/pull/28297/checks?check_run_id=25366990159.

[Hecatia-Lapislazuli]

Ah, hmm... okay missed that as well. Thanks for pointing it out to me. Not sure how to address that...

Could change logic to match the isLinear function for the parser? Just not sure that would handle all edge cases... Or work. Also might be worth moving logic to OsuSelectionScaleHandler.scaleSlider instead. I'll look into it.

[bdach]

I mean yes you could change to match but I would not like to see two implementations of the same thing. So either it should be split to a helper or moved to some common place that covers both of these uses (if possible, not sure).

Notably, this also exists:

osu/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/PathControlPointVisualiser.cs

Lines 122 to 124 in c7a89c3

	RectangleF boundingBox = PathApproximator.CircularArcBoundingBox(points);
	if (boundingBox.Width >= 640 || boundingBox.Height >= 480)
	first.Type = PathType.BEZIER;

[Hecatia-Lapislazuli]

I believe i found a decent solution by just using the ThetaRange in CircularArcProperties - if it is sufficiently close to zero, the radius of the arc needed to have more than a floating point difference is massive.

Currently just using within 0.0005, which corresponds with a 0.001 change in X, and a change of at least 4 in Y.

I've also reworked the test to check that the conversion to a Bezier curve works, but I feel a further test to ensure this won't affect current beatmaps is a decent idea.

I'm going to head to bed for now though, took longer than I thought...

[Hecatia-Lapislazuli]

Checked properly, isLinear doesn't stop the OOM issue (which is a surprise to me), nor does the logic in PathControlPointVisualiser stop the OOM.

Will convert to draft for now, don't think I'll have a solution that handles the edge cases for a while (unless i restrict the patch just to the editor, which only partially solves the issue).

I will bring up one idea I thought of though, of just modifying the following in framework, so an argument can cap the maximum list of vectors to a sane amount.

int amountPoints = 2 * pr.Radius <= circular_arc_tolerance ? 2 : Math.Max(2, (int)Math.Ceiling(pr.ThetaRange / (2 * Math.Acos(1 - circular_arc_tolerance / pr.Radius))));

https://github.com/ppy/osu-framework/blob/efb30d91a1071986b1688dc86f6f28b9531c6219/osu.Framework/Utils/PathApproximator.cs#L186

In this case, most likely 2^12, which corresponds to amount of points needed to fill a half circle at a radius of 10^6. Here's a desmos link for the numbers.

Would like your opinion on this if possible, just with its viability.

[Hecatia-Lapislazuli]

Every time I feel i've checked for plausible maintainability issues, a new one crops up, hah... Welp. CodeFactor's been addressed now with that last commit, at any rate.

Added a test to ensure that when a path type is PERFECT_CURVE, that it'll match the theoretical perfect curve - except for certain cases already handled elsewhere that i've documented in the test script.

Also ignoring issues that crop up from interpolation, so we only check the points where a perfect curve would have a point. I don't feel like it's worthwhile adding, given how it's implemented, but I can do that as well if one would want that to be tested for.

Anywho, after all that has been said and done, with me spending way too many hours on it... I think the easiest solution is just checking if the amount of subpoints we'd have is int.MaxValue in calculateSubPath, it's a safe operation to preform without worrying about changing current beatmaps too, as if the value is int.MaxValue, we'd end up having a crash anyways from the game trying to allocate 2^31 points into memory, which will be over ~100gb of memory. (assuming a Vector2 uses at least 64 bits)

I'll add another commit to change this, and I think should actually be ready then for review, @bdach

[bdach]

I'll add another commit to change this, and I think should actually be ready then for review, @bdach

I'm confused, is that commit already in or not? The PR was marked ready for review but I only see 2 commits prior to this remark...

[Hecatia-Lapislazuli]

Sorry, wrote the message poorly. It is in, the change was commit 6c4def1

[bdach]

Yeah so this looks flimsy at best to me (but so does the framework implementation of the same thing, for that matter).

• Why is int.MaxValue the bound here? You could probably just as easily cause an OOM with half of that number of points. I feel like this bound should be much smarter than that.
• There's an (int) cast in that expression there. What guarantees that it doesn't overflow? And yes if it does, then the Math.Max(2, ...) t hing probably saves this, but it also makes the huge arc into a segment which also seems not correct.

[Hecatia-Lapislazuli]

Would agree with it being somewhat flimsy, but just copying framework is the simplest way I could think of to solve this without potential for edge cases.

From testing, seems to be either a reasonable number or int.MaxValue, with nothing really in between.

I believe this is as for when the radius is greater than ~3 million, arccos(1-0.1/radius) will be less than 0.0003.
Due to a floating point error, this results in arccos returning 1/double.MaxValue (presumably, but either way, a value less than 1/int.MaxValue). Then we divide ThetaRange by this value, which results in a value above int.MaxValue. When we cast this double to an integer value, as it's greater than the integer limit, it's instead set to the max value.

Which... does feel very jank, thinking about it. But Framework is going through the same calculation, and we're just making sure that we can't calculate a perfect curve with int.MaxValue subpoints regardless regardless. If it underflows, framework will underflow as well, which also works out fine... ish.

This is also explanation for why it's meaningless to have a lower bound - 12000 sub points is about the maximum you can have without said floating point error.

[bdach]

All of the above may be well and true but it's still relying on a bunch of internal implementation details of fp math and arccos and so on. A sensibly defined bound protects against any of that changing around.

[Hecatia-Lapislazuli]

Ah wait misread your comment initially... Okay, yeah I'd agree. Hmm... did mention 12000 is theoretical max, but probably should be lower still.

I'll go with 1000, we'd need an arc length of greater than ~20 thousand to surpass this, which should be impossible for ranked beat maps anyways.
desmos link

[bdach]

@smoogipoo can you look at this thing and chip in as to whether you think this sort of check should live in game or maybe it should go live in framework before I start queueing diffcalcs and such to check for regressions?

[smoogipoo]

I think this is fine to live in the game

[bdach]

!diffcalc

[github-actions]

Target: #28297
Spreadsheet: https://docs.google.com/spreadsheets/d/1373FDOWExQZD09kXx4wCTBuu0XrV4iwqZTEwkpawFjA/edit

[bdach]

Well sheets look empty at least, which is good...

Notably in testing I can only hit this in slider configurations like these:

Which is funny because the other logic that should be handling this is already reverting the perfect curve node to bezier, but somehow the path approximator is still happily estimating arcs. Probably has something to do with the convoluted invalidation flow in slider path, and I'm not sure whether it can actually be fixed locally there, so I think I'd be somewhat fine with this solution... if not for the inline comments:

[bdach]

would prefer a second approval here before proceeding with this