Flipping a slider can change curve type of PerfectCurve control points to Bezier

[OliBomby]

Type

Game behaviour

Bug description

When editing a beatmap, flipping a PerfectCurve slider may change the curve type of that slider to Bezier. It's not just flipping a slider that can produce this behaviour, rotating, reversing, and possibly more operations can do this. I've done some investigation as to why this happens.

The code which flips the position of the slider control points moves the control points one by one.

osu/osu.Game.Rulesets.Osu/Edit/OsuSelectionHandler.cs

Lines 130 to 136 in 4627e4f

	foreach (var point in slider.Path.ControlPoints)
	{
	point.Position = new Vector2(
	(direction == Direction.Horizontal ? -1 : 1) * point.Position.X,
	(direction == Direction.Vertical ? -1 : 1) * point.Position.Y
	);
	}

Each time an anchor is moved, the path is invalidated and the PathControlPointPiece gets notified and calls updatePathType

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

Lines 224 to 226 in 4627e4f

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

If not all control points have been moved yet and they happen to be in an unfortunate position, then we can get an invalid perfect curve which gets corrected to a bezier.

Screenshots or videos

osu._hVcl7UwaMT.mp4

Version

latest master branch 18/8/2022

Logs

runtime.log

[peppy]

Thanks for the detailed report. Definitely seems like either we want to be invalidating less often (ie. not watching the collection and triggering a higher level event) or updating all points in one go somehow.

[smoogipoo]

Other places (OsuModHardRock) update all points in one go. Can/should do that here also.

[peppy]

For reference:

osu/osu.Game.Rulesets.Osu/Utils/OsuHitObjectGenerationUtils.cs

Lines 147 to 151 in f8830c6

	var controlPoints = slider.Path.ControlPoints.Select(p => new PathControlPoint(p.Position, p.Type)).ToArray();
	foreach (var point in controlPoints)
	point.Position = new Vector2(point.Position.X, -point.Position.Y);
	slider.Path = new SliderPath(controlPoints, slider.Path.ExpectedDistance.Value);

[OliBomby]

I think it will be beneficial to add some feature to SliderPath which can temporarily prevent it from invalidating. Perhaps a beginChange() and endChange() method which disables invalidating until endChange() is called and only then invalidates once. This would allow you to make changes to slider paths like moving points one by one without triggering invalidation while the edit is not yet complete. Performance would be better because you won't have to create multiple temporary arrays to edit all control points at once. It might also be easier to understand for the reader because seeing beginChange() and endChange() explains to the reader why it is being done in that way.

[bdach]

I don't really agree with this "prevent invalidation" flow. To me the fact that the state of the slider is always consistent with the currently-set path is a feature which decreases complexity (because you can't forget to call endChange() and leave the slider in a broken state). I don't see any real reason to add more {begin,end}Change() stuff if setting the flipped path in one go is a viable alternative.

This would allow you to make changes to slider paths like moving points one by one without triggering invalidation while the edit is not yet complete

I'm not sure where this would be wanted. At least when editing interactively, again, having the state of the path update as modifications are applied is the desirable outcome.