Undo or re-opening the map causes sliders with more than 3 nodes to reset all the modified nodes back to Bezier

[RaiLTC]

Example of undo on a slider with 3 nodes and more than 3 nodes

Happens both on regular right click delete and SHIFT right click delete

osu!lazer version: 2021.220.0

[peppy]

This is part of the automatic logic that decides you've used the wrong curve type based on the number of points available to be consumed by the curve. It can happy when you have a "perfect curve" which doesn't have 3 points (the only number of points which will work correctly for a perfect curve), or if you have three points in a completely straight line.

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

Lines 312 to 322 in 2150cf1

	// Edge-case rules (to match stable).
	if (type == PathType.PerfectCurve)
	{
	if (vertices.Length != 3)
	type = PathType.Bezier;
	else if (isLinear(vertices))
	{
	// osu-stable special-cased colinear perfect curves to a linear path
	type = PathType.Linear;
	}
	}

This logic is run on undo/redo/load.

I guess from a usability perspective, this may not be what we want. But the fallbacks are there to get the user in a good state, so they aren't trying to use an incorrect curve type and not getting any visible result.

[bdach]

Watching the video, I'm a bit confused by the state even before the undo. It doesn't really make sense to allow a perfect path type if there are four path nodes, since it is incredibly likely that a circular arc cannot be physically drawn through those points.

The way I see it, the addition of a fourth point to a circular arc should terminate the arc and begin a new path segment, with a completely different path type (linear is the obvious choice, I guess?) While it might look a bit wonky UX-wise, it sort of is a lie anyway to pretend the path is a perfect curve with 4 points, so maybe best to be explicit about that...?

[bdach]

How does something like this video feel, UX-wise? This is the demonstration of a crude implementation of the logic I proposed above that would verify the correctness of applied curve types on path changes. This would therefore resolve the issue for new maps by making it impossible to save a map in an invalid state, as it seems like stable cannot generate a map with a broken perfect slider.

I know it's a bit wonky (and I still haven't fixed all of the edge cases - for example, the Perfect curve type should be disabled in the context menu if the next 2 nodes can't be used for it), but I wanted to get a feel out whether this feels acceptable. I think it's okay - it is hard to understand, but so is the magical fixup of perfect into Bezier, I feel.

Edit: Crude diff here, if you'd like to fast-forward this, since I probably won't be able to clean this up today:

diff --git a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/PathControlPointVisualiser.cs b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/PathControlPointVisualiser.cs
index ce5dc4855e..f98ce2e546 100644
--- a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/PathControlPointVisualiser.cs
+++ b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/Components/PathControlPointVisualiser.cs
@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using System.Collections.Specialized;
+using System.Diagnostics;
 using System.Linq;
 using Humanizer;
 using osu.Framework.Allocation;
@@ -115,6 +116,41 @@ private void onControlPointsChanged(object sender, NotifyCollectionChangedEventA
 
                     break;
             }
+
+            verifyNodeTypes();
+        }
+
+        private void verifyNodeTypes()
+        {
+            int previousSegmentStartIndex = 0;
+
+            Debug.Assert(controlPoints[0].Type.Value != null);
+            PathType segmentType = controlPoints[0].Type.Value.Value;
+
+            int i;
+
+            for (i = 1; i < controlPoints.Count; ++i)
+            {
+                var piece = controlPoints[i];
+                var pieceType = piece.Type.Value;
+
+                if (pieceType != null)
+                {
+                    checkPreviousSegment(segmentType, previousSegmentStartIndex, i);
+
+                    previousSegmentStartIndex = i;
+                    segmentType = pieceType.Value;
+                }
+            }
+
+            checkPreviousSegment(segmentType, previousSegmentStartIndex, i - 1);
+        }
+
+        private void checkPreviousSegment(PathType segmentType, int segmentStart, int segmentEnd)
+        {
+            int segmentLength = segmentEnd - segmentStart + 1;
+            if (segmentType == PathType.PerfectCurve && segmentLength != 3)
+                controlPoints[segmentStart].Type.Value = segmentLength > 3 ? PathType.Bezier : PathType.Linear;
         }
 
         protected override bool OnClick(ClickEvent e)
@@ -219,6 +255,8 @@ private MenuItem createMenuItemForPathType(PathType? type)
             {
                 foreach (var p in Pieces.Where(p => p.IsSelected.Value))
                     p.ControlPoint.Type.Value = type;
+
+                verifyNodeTypes(); // TODO: block options that don't make sense instead
             });
 
             if (countOfState == totalCount)

[peppy]

I think it's fine. The curve guessing happening only during initial placement makes sense to me. After it is placed, the curve types should be set in stone.

[peppy]

With the recent UX changes to slider placement I believe this is no longer an issue. Note that undo/redo state handling is still a bit haphazard so if you try to reproduce this make sure to move the slider between each node change (this remaining issue is being tracked via #11901).