Random mod - broken difficulty calculation

[altrevamp]

Type

Game behaviour

Bug description

Certain maps, usually around 5* range, gain a very high difficulty increase when selecting the Random mod. Sometimes it can jump up to 7-8* like in the first video provided.

The actual difficulty of the map with Random is clearly not 8* (shown in second video).

Screenshots or videos

2024-09-06.15-27-12.mp4

2024-09-06.15-27-44.mp4

Version

2024.906.1

Logs

compressed-logs.zip

[cihe13375]

For some reason, other mods that usually don't change the difficulty rating (hidden, traceable, autoplay) can increase or decrease the difficulty as well, also shown in the first video.

This is not a new behavior, I think it happens because the seed is recalculated when selecting a mod (if you manually specify a seed the SR would not change with e.g. HD anymore)

The high SR itself is likely a bug though, or at least the last release does not behave like this

[Joehuu]

For some reason, other mods that usually don't change the difficulty rating (hidden, traceable, autoplay) can increase or decrease the difficulty as well, also shown in the first video.

That is a duplicate of #13453.

Leaving this open for the other issue.

[altrevamp]

I found that applying the Strict Tracking mod as well seems to correct the difficulty.

Edit: This led me to finding out that fast reverse sliders (and buzz sliders) cause a massive increase in pp. This might be causing the issue. First video shows a more extreme example of this, and the second video shows two reverse sliders adding 200+ pp.

2024-09-07.01-21-21.mp4

2024-09-07.01-26-43.mp4

[huyenden]

Yeah I just played the random mod like you a few minutes ago and the star difficulty and pp reward increased which shocked me even though I knew it was a bug. That beatmap was only a 3 star map. Will probably just temporarily consider random a bug for a while.

[bdach]

This is a regression which bisects to f7cb6b9, specifically this change:

osu/osu.Game.Rulesets.Osu/Objects/Slider.cs

Lines 257 to 258 in ccf1acc

	if (LastRepeat != null)
	LastRepeat.Position = RepeatCount % 2 == 0 ? Position : Position + Path.PositionAt(1);

This breaks random specifically because random was doing its own things to attempt to compensate for the fact that moving the parent object did not adjust any other nesteds other than the head or tail:

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

Line 235 in ccf1acc

shiftNestedObjects(slider, workingObject.PositionModified - workingObject.PositionOriginal);

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

Lines 310 to 324 in ccf1acc

	/// <summary>
	/// Shifts all nested <see cref="SliderTick"/>s and <see cref="SliderRepeat"/>s by the specified shift.
	/// </summary>
	/// <param name="slider"><see cref="Slider"/> whose nested <see cref="SliderTick"/>s and <see cref="SliderRepeat"/>s should be shifted</param>
	/// <param name="shift">The <see cref="Vector2"/> the <see cref="Slider"/>'s nested <see cref="SliderTick"/>s and <see cref="SliderRepeat"/>s should be shifted by</param>
	private static void shiftNestedObjects(Slider slider, Vector2 shift)
	{
	foreach (var hitObject in slider.NestedHitObjects.Where(o => o is SliderTick || o is SliderRepeat))
	{
	if (!(hitObject is OsuHitObject osuHitObject))
	continue;
	osuHitObject.Position += shift;
	}
	}

Thus, both the slider and random mod attempted to to their thing after the slider was moved, but ended up combining to do the wrong thing.

This is corroborated by applying the following diff:

diff --git a/osu.Game.Rulesets.Osu/Utils/OsuHitObjectGenerationUtils_Reposition.cs b/osu.Game.Rulesets.Osu/Utils/OsuHitObjectGenerationUtils_Reposition.cs
index a9ae313a31..d325a9c62a 100644
--- a/osu.Game.Rulesets.Osu/Utils/OsuHitObjectGenerationUtils_Reposition.cs
+++ b/osu.Game.Rulesets.Osu/Utils/OsuHitObjectGenerationUtils_Reposition.cs
@@ -319,6 +319,9 @@ private static void shiftNestedObjects(Slider slider, Vector2 shift)
                 if (!(hitObject is OsuHitObject osuHitObject))
                     continue;
 
+                if (hitObject == slider.NestedHitObjects.OfType<SliderRepeat>().LastOrDefault())
+                    continue;
+
                 osuHitObject.Position += shift;
             }
         }

which fixes the star rating (disclaimer: to get reliable reproduction I was testing on a single map, https://osu.ppy.sh/beatmapsets/1892718#osu/3903975, with a single fixed seed of 1337 - but I don't believe there is any reason to believe that the other cases reported don't pertain to the same scenario).

I have two actual proposals of fixes. Maybe even you could combine them into one total fix.

The safe one

In retrospect that Slider change is obviously horrible and I feel like an idiot for letting it pass review. Updating the position of only the last repeat is insane. And it's seemingly not even needed when you can write

diff --git a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderCircleOverlay.cs b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderCircleOverlay.cs
index 9c2998466a..83278660d4 100644
--- a/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderCircleOverlay.cs
+++ b/osu.Game.Rulesets.Osu/Edit/Blueprints/Sliders/SliderCircleOverlay.cs
@@ -63,7 +63,7 @@ protected override void Update()
             base.Update();
 
             var circle = position == SliderPosition.Start ? (HitCircle)slider.HeadCircle :
-                slider.RepeatCount % 2 == 0 ? slider.TailCircle : slider.LastRepeat!;
+                slider.RepeatCount % 2 == 0 ? slider.TailCircle : slider.HeadCircle;
 
             CirclePiece.UpdateFrom(circle);
             marker?.UpdateFrom(circle);
diff --git a/osu.Game.Rulesets.Osu/Objects/Slider.cs b/osu.Game.Rulesets.Osu/Objects/Slider.cs
index 2b3bb18844..77e821fe7c 100644
--- a/osu.Game.Rulesets.Osu/Objects/Slider.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Slider.cs
@@ -253,9 +253,6 @@ private void updateNestedPositions()
 
             if (TailCircle != null)
                 TailCircle.Position = EndPosition;
-
-            if (LastRepeat != null)
-                LastRepeat.Position = RepeatCount % 2 == 0 ? Position : Position + Path.PositionAt(1);
         }
 
         protected void UpdateNestedSamples()

which just gets rid of all that bad crap entirely and I have no reason to believe it wouldn't work.

The less safe one

The fact that that updateNestedPositions() flow doesn't touch all nested objects is also obviously bad. It can be improved, but the prerequisite would be storing the PathProgress of the nesteds. At this point the random mod hacks go away too:

diff --git a/osu.Game.Rulesets.Osu/Objects/Slider.cs b/osu.Game.Rulesets.Osu/Objects/Slider.cs
index 2b3bb18844..84bae75602 100644
--- a/osu.Game.Rulesets.Osu/Objects/Slider.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Slider.cs
@@ -204,6 +204,7 @@ protected override void CreateNestedHitObjects(CancellationToken cancellationTok
                             SpanStartTime = e.SpanStartTime,
                             StartTime = e.Time,
                             Position = Position + Path.PositionAt(e.PathProgress),
+                            PathProgress = e.PathProgress,
                             StackHeight = StackHeight,
                         });
                         break;
@@ -236,6 +237,7 @@ protected override void CreateNestedHitObjects(CancellationToken cancellationTok
                             StartTime = StartTime + (e.SpanIndex + 1) * SpanDuration,
                             Position = Position + Path.PositionAt(e.PathProgress),
                             StackHeight = StackHeight,
+                            PathProgress = e.PathProgress,
                         });
                         break;
                 }
@@ -254,8 +256,11 @@ private void updateNestedPositions()
             if (TailCircle != null)
                 TailCircle.Position = EndPosition;
 
-            if (LastRepeat != null)
-                LastRepeat.Position = RepeatCount % 2 == 0 ? Position : Position + Path.PositionAt(1);
+            foreach (var repeat in NestedHitObjects.OfType<SliderRepeat>())
+                repeat.Position = Position + Path.PositionAt(repeat.PathProgress);
+
+            foreach (var tick in NestedHitObjects.OfType<SliderTick>())
+                tick.Position = Position + Path.PositionAt(tick.PathProgress);
         }
 
         protected void UpdateNestedSamples()
diff --git a/osu.Game.Rulesets.Osu/Objects/SliderRepeat.cs b/osu.Game.Rulesets.Osu/Objects/SliderRepeat.cs
index e95cfd369d..1bbd1e8070 100644
--- a/osu.Game.Rulesets.Osu/Objects/SliderRepeat.cs
+++ b/osu.Game.Rulesets.Osu/Objects/SliderRepeat.cs
@@ -5,6 +5,8 @@ namespace osu.Game.Rulesets.Osu.Objects
 {
     public class SliderRepeat : SliderEndCircle
     {
+        public double PathProgress { get; set; }
+
         public SliderRepeat(Slider slider)
             : base(slider)
         {
diff --git a/osu.Game.Rulesets.Osu/Objects/SliderTick.cs b/osu.Game.Rulesets.Osu/Objects/SliderTick.cs
index 74ec4d6eb3..219c2be00b 100644
--- a/osu.Game.Rulesets.Osu/Objects/SliderTick.cs
+++ b/osu.Game.Rulesets.Osu/Objects/SliderTick.cs
@@ -13,6 +13,7 @@ public class SliderTick : OsuHitObject
     {
         public int SpanIndex { get; set; }
         public double SpanStartTime { get; set; }
+        public double PathProgress { get; set; }
 
         protected override void ApplyDefaultsToSelf(ControlPointInfo controlPointInfo, IBeatmapDifficultyInfo difficulty)
         {
diff --git a/osu.Game.Rulesets.Osu/Utils/OsuHitObjectGenerationUtils_Reposition.cs b/osu.Game.Rulesets.Osu/Utils/OsuHitObjectGenerationUtils_Reposition.cs
index a9ae313a31..f7a0543739 100644
--- a/osu.Game.Rulesets.Osu/Utils/OsuHitObjectGenerationUtils_Reposition.cs
+++ b/osu.Game.Rulesets.Osu/Utils/OsuHitObjectGenerationUtils_Reposition.cs
@@ -232,8 +232,6 @@ private static Vector2 clampSliderToPlayfield(WorkingObject workingObject)
             slider.Position = workingObject.PositionModified = new Vector2(newX, newY);
             workingObject.EndPositionModified = slider.EndPosition;
 
-            shiftNestedObjects(slider, workingObject.PositionModified - workingObject.PositionOriginal);
-
             return workingObject.PositionModified - previousPosition;
         }
 
@@ -307,22 +305,6 @@ public static RectangleF CalculatePossibleMovementBounds(Slider slider)
             return new RectangleF(left, top, right - left, bottom - top);
         }
 
-        /// <summary>
-        /// Shifts all nested <see cref="SliderTick"/>s and <see cref="SliderRepeat"/>s by the specified shift.
-        /// </summary>
-        /// <param name="slider"><see cref="Slider"/> whose nested <see cref="SliderTick"/>s and <see cref="SliderRepeat"/>s should be shifted</param>
-        /// <param name="shift">The <see cref="Vector2"/> the <see cref="Slider"/>'s nested <see cref="SliderTick"/>s and <see cref="SliderRepeat"/>s should be shifted by</param>
-        private static void shiftNestedObjects(Slider slider, Vector2 shift)
-        {
-            foreach (var hitObject in slider.NestedHitObjects.Where(o => o is SliderTick || o is SliderRepeat))
-            {
-                if (!(hitObject is OsuHitObject osuHitObject))
-                    continue;
-
-                osuHitObject.Position += shift;
-            }
-        }
-
         /// <summary>
         /// Clamp a position to playfield, keeping a specified distance from the edges.
         /// </summary>

This is rather untested (likely needs a full diffcalc run for a proper check), and there is a chance something else can go bad here, which is why I'm calling this less safe.

@ppy/team-client requesting feedback on the above re: your preferences. Maybe @OliBomby has something to say here too.

As a last thing I really think we should get some sort of automated scrutiny on all hitobject classes because it's the second time in recent memory that an editor change causes a diffcalc fail. Maybe a CODEOWNERS on the hitobject classes for automated review, maybe some github workflow to automatically run on any and all changes to hitobject classes.

cc @ppy/osu-pp-committee

[OliBomby]

var circle = position == SliderPosition.Start ? (HitCircle)slider.HeadCircle :
                slider.RepeatCount % 2 == 0 ? slider.TailCircle : slider.HeadCircle;

This wouldn't work because when the repeat count is even, the tail circle and head circle are in the same place.

I prefer your 'less safe' solution.

[peppy]

I think I prefer the latter proposal as it is more likely to handle similar cases in the future.