Use stable sort for catch hyperdash generation

[ekrctb]

Potentially affecting difficulty calculation, but should only affect aspire maps. Requesting @smoogipoo for checking.
Example of change: https://osu.ppy.sh/beatmapsets/490223#fruits/1044929 00:51:269 which fruit will be hyper.

Motivation for this change: It is important to use the consistent ordering of palpable objects. Otherwise, the hyperdash target may not be the next palpable object, which should be impossible (not worth having as an edge case).
For consistent ordering, the stably-sorted ordering is better than List.Sort because it is more canonical.
Currently, the difficulty calculator uses OrderBy. The auto generator isn't currently sorting the hit objects but it will be fixed to sort objects.

[bdach]

If the stability of the sort is important, then there should be an inline comment about it or a test covering it. Or both. Otherwise there's no transparent reason why this particular one way to sort out of the multiple ones offered by the standard library is used here precisely.

[smoogipoo]

This fixes it visually, but gameplay is still broken (actually, even more broken), likely because of a comparer somewhere...

Spreadsheet: https://docs.google.com/spreadsheets/d/1zcwKXKow_JZrW9KOPY1-RzIhtUXviZfZVyML8ftYLnA/edit#gid=679340096

[bdach]

@smoogipoo do you recall what is broken on this branch specifically? I just checked it out to see if I can spot it myself and maybe fix it but I'm not sure I can see, which means I'm not sure if I can fix.

[smoogipoo]

Oh, didn't notice the spreadsheet was dead. I'll rerun it.

[bdach]

Sheets are sheets, but you mentioned "gameplay being broken" above despite visuals looking correct, and I'm not sure what that's referring to.

Unless that is referring to the sheets...? As in, star rating is still wrong?

[smoogipoo]

I didn't remember at the time, but I remember now. The actual mentioned beatmap and pattern in the issue description here is broken, because it cancels hyperdash immediately.

[smoogipoo]

Updated spreadsheet https://docs.google.com/spreadsheets/d/17CnYDxK_LvmdwxTd4H0m4lvkMgG1ake_LjvbD0cvPGs/edit#gid=1197932367

But not useful in resolving the above issue, as mentioned.

[smoogipoo]

Just to demonstrate...

Master:

2023-10-05.14-26-10.mp4

This PR:

2023-10-05.14-25-00.mp4

Notice how both are incorrect, but master allows the hyper on the last jump. That's what I mean by breaking gameplay even more.

[bdach]

After several hours of investigation, I'm not sure what to do with this PR.

likely because of a comparer somewhere

This intuition was correct, but the resolution is difficult - because the comparer in question is this one:

osu/osu.Game/Rulesets/UI/HitObjectContainer.cs

Lines 179 to 187 in e8d0962

	protected override int Compare(Drawable x, Drawable y)
	{
	if (!(x is DrawableHitObject xObj) || !(y is DrawableHitObject yObj))
	return base.Compare(x, y);
	// Put earlier hitobjects towards the end of the list, so they handle input first
	int i = yObj.HitObject.StartTime.CompareTo(xObj.HitObject.StartTime);
	return i == 0 ? CompareReverseChildID(x, y) : i;
	}

Because the comparer works by start time, it will return zero for the doubled-up fruit, and fall back to reverse child ID. This causes the following ordering of AliveInternalChildren:

This is important, as this ordering determines the subsequent ordering of applying results / judgements. In this case, the hyperfruit will be judged first, and the normal fruit will be judged second. This fact causes the hyperdash to be immediately canceled.

Okay, you could say, we can fix this, right? I did, and applied this:

diff --git a/osu.Game.Rulesets.Catch/UI/CatchHitObjectContainer.cs b/osu.Game.Rulesets.Catch/UI/CatchHitObjectContainer.cs
new file mode 100644
index 0000000000..b84097e113
--- /dev/null
+++ b/osu.Game.Rulesets.Catch/UI/CatchHitObjectContainer.cs
@@ -0,0 +1,26 @@
+// Copyright (c) ppy Pty Ltd <[email protected]>. Licensed under the MIT Licence.
+// See the LICENCE file in the repository root for full licence text.
+
+using osu.Framework.Graphics;
+using osu.Game.Rulesets.Catch.Objects.Drawables;
+using osu.Game.Rulesets.UI.Scrolling;
+
+namespace osu.Game.Rulesets.Catch.UI
+{
+    public partial class CatchHitObjectContainer : ScrollingHitObjectContainer
+    {
+        protected override int Compare(Drawable x, Drawable y)
+        {
+            if (!(x is DrawablePalpableCatchHitObject xObj) || !(y is DrawablePalpableCatchHitObject yObj))
+                return base.Compare(x, y);
+
+            // Put earlier hitobjects towards the end of the list, so they handle input first
+            int i = yObj.HitObject.StartTime.CompareTo(xObj.HitObject.StartTime);
+
+            if (i == 0)
+                i = -yObj.HyperDash.Value.CompareTo(xObj.HyperDash.Value);
+
+            return i == 0 ? CompareReverseChildID(x, y) : i;
+        }
+    }
+}
diff --git a/osu.Game.Rulesets.Catch/UI/CatchPlayfield.cs b/osu.Game.Rulesets.Catch/UI/CatchPlayfield.cs
index f091dee845..7fc60f7622 100644
--- a/osu.Game.Rulesets.Catch/UI/CatchPlayfield.cs
+++ b/osu.Game.Rulesets.Catch/UI/CatchPlayfield.cs
@@ -51,6 +51,8 @@ public CatchPlayfield(IBeatmapDifficultyInfo difficulty)
             this.difficulty = difficulty;
         }
 
+        protected override ScrollingHitObjectContainer CreateScrollingHitObjectContainer() => new CatchHitObjectContainer();
+
         protected override GameplayCursorContainer CreateCursor() => new CatchCursorContainer();
 
         [BackgroundDependencyLoader]

The basic idea is, if hyperfruit are an issue, let's try to evaluate them last, right? Wrong - this doesn't fully fix it. And that is because of juice streams.

Juice streams have nested fruit. You can probably see where this is going. The comparer doesn't work for that, as when comparing objects, it will see a juice stream and a fruit, and as the juice stream isn't "palpable" by itself, it will fall back to the base comparer. Thus, if you have a hyperfruit at the same time as a normal fruit that ends a juice stream, the hyperfruit will be updated / judged first, and then the juice stream will, which only at this point will contain the end fruit as a child, instantly canceling the hyperdash again.

Now this all is wrapped under the daisy-chain of OnNewResult callbacks and events, which means that reordering anything here is basically infeasible. The only way I can imagine getting this to work is via some kind of hack local to Catcher, i.e. "if hyperdash was handled at this exact time instant already, just keep going where you were going to go" - but that's ugly, and I question if it's even worth it for an aspire map? I will however say that it feels quite bad that we cannot get this to work easily...

@ppy/team-client thoughts would be appreciated.

[smoogipoo]

I don't know how to resolve this without more comparer hackery.

[bdach]

I mean the point was moreso that I don't see any feasible comparer hackery here. It's not possible to enforce that a fruit wedged inside a juice stream should be judged before a hyperfruit via the hitobject container comparer alone, because the juice stream fruit is not a direct child of the hitobject container. It will not get compared like that. And dynamically changing the order can't be done because you'd have to somehow re-sort AliveInternalChildren.

Something that appears to work is something along the lines of

diff --git a/osu.Game.Rulesets.Catch/UI/Catcher.cs b/osu.Game.Rulesets.Catch/UI/Catcher.cs
index f77dab56c8..716d881d4d 100644
--- a/osu.Game.Rulesets.Catch/UI/Catcher.cs
+++ b/osu.Game.Rulesets.Catch/UI/Catcher.cs
@@ -118,6 +118,7 @@ public CatcherAnimationState CurrentState
 
         private Color4 hyperDashColour = DEFAULT_HYPER_DASH_COLOUR;
 
+        private double? lastHyperDashStartTime;
         private double hyperDashModifier = 1;
         private int hyperDashDirection;
         private float hyperDashTargetPosition;
@@ -230,16 +231,23 @@ public void OnNewResult(DrawableCatchHitObject drawableObject, JudgementResult r
             // droplet doesn't affect the catcher state
             if (hitObject is TinyDroplet) return;
 
-            if (result.IsHit && hitObject.HyperDashTarget is CatchHitObject target)
+            // if a hyper fruit was already handled this frame, just go where it says to go.
+            // this special-cases some aspire maps that have doubled-up objects (one hyper, one not).
+            // handling this "properly" elsewhere is impossible as there is no feasible way to ensure
+            // the hyperfruit gets judged second (especially if it coincides with a last fruit in a juice stream).
+            if (lastHyperDashStartTime != Time.Current)
             {
-                double timeDifference = target.StartTime - hitObject.StartTime;
-                double positionDifference = target.EffectiveX - X;
-                double velocity = positionDifference / Math.Max(1.0, timeDifference - 1000.0 / 60.0);
+                if (result.IsHit && hitObject.HyperDashTarget is CatchHitObject target)
+                {
+                    double timeDifference = target.StartTime - hitObject.StartTime;
+                    double positionDifference = target.EffectiveX - X;
+                    double velocity = positionDifference / Math.Max(1.0, timeDifference - 1000.0 / 60.0);
 
-                SetHyperDashState(Math.Abs(velocity) / BASE_DASH_SPEED, target.EffectiveX);
+                    SetHyperDashState(Math.Abs(velocity) / BASE_DASH_SPEED, target.EffectiveX);
+                }
+                else
+                    SetHyperDashState();
             }
-            else
-                SetHyperDashState();
 
             if (result.IsHit)
                 CurrentState = hitObject.Kiai ? CatcherAnimationState.Kiai : CatcherAnimationState.Idle;
@@ -289,6 +297,8 @@ public void SetHyperDashState(double modifier = 1, float targetPosition = -1)
 
                 if (wasHyperDashing)
                     runHyperDashStateTransition(false);
+
+                lastHyperDashStartTime = null;
             }
             else
             {
@@ -298,6 +308,8 @@ public void SetHyperDashState(double modifier = 1, float targetPosition = -1)
 
                 if (!wasHyperDashing)
                     runHyperDashStateTransition(true);
+
+                lastHyperDashStartTime = Time.Current;
             }
         }
 

but that feels borderline too ugly to live...

[smoogipoo]

The way stable handled this is by fruits not being nested in parenting objects. I think what you have there is fine personally. It's an edge-case that'll only happen in aspire / "2B" maps.

[bdach]

Okay well I've applied the patch with test coverage. Please double check that it behaves as you'd expect it to.