Force new combo on objects succeeding a break

[bdach]

No issue thread for this again. Reported internally on discord.

Placing this logic in the beatmap processor, as a post-processing step, means that the new combo force won't be visible until a placement has been committed. That can be seen as subpar, but I tried putting this logic in the placement and it sucked anyway:

• While the combo number was correct, the colour looked off, because it would use the same combo colour as the already-placed objects after said break, which would only cycle to the next, correct one on placement
• Not all scenarios can be handled in the placement. Refer to one of the test cases added in 973f606, wherein two objects are placed far apart from each other, and an automated break is inserted between them - the placement has no practical way of knowing whether it's going to have a break inserted automatically before it or not.

If you wanna check out the original version I tried and see why I didn't end up PRing it, see diff below:

diff --git a/osu.Game/Screens/Edit/Compose/Components/ComposeBlueprintContainer.cs b/osu.Game/Screens/Edit/Compose/Components/ComposeBlueprintContainer.cs
index 0ffd1072cd..2d340a08a4 100644
--- a/osu.Game/Screens/Edit/Compose/Components/ComposeBlueprintContainer.cs
+++ b/osu.Game/Screens/Edit/Compose/Components/ComposeBlueprintContainer.cs
@@ -46,6 +46,9 @@ public partial class ComposeBlueprintContainer : EditorBlueprintContainer
         [Resolved(canBeNull: true)]
         private EditorScreenWithTimeline editorScreen { get; set; }
 
+        [Resolved]
+        private EditorBeatmap editorBeatmap { get; set; }
+
         /// <remarks>
         /// Positional input must be received outside the container's bounds,
         /// in order to handle composer blueprints which are partially offscreen.
@@ -173,8 +176,14 @@ private void nudgeSelection(Vector2 delta)
 
         private void updatePlacementNewCombo()
         {
-            if (CurrentHitObjectPlacement?.HitObject is IHasComboInformation c)
-                c.NewCombo = NewCombo.Value == TernaryState.True;
+            var hitObject = CurrentHitObjectPlacement?.HitObject;
+            if (hitObject is not IHasComboInformation c)
+                return;
+
+            var closestBreak = editorBeatmap.Breaks.LastOrDefault(b => b.StartTime <= hitObject.StartTime);
+            bool isFirstAfterBreak = closestBreak != null && !editorBeatmap.HitObjects.Any(ho => ho.StartTime >= closestBreak.EndTime && ho.StartTime <= hitObject.StartTime);
+
+            c.NewCombo = NewCombo.Value == TernaryState.True || isFirstAfterBreak;
         }
 
         private void updatePlacementSamples()
@@ -364,6 +373,7 @@ protected override void Update()
 
                 // updates the placement with the latest editor clock time.
                 updatePlacementTimeAndPosition();
+                updatePlacementNewCombo();
             }
         }
 

[peppy]

I think applying as a post-process is fine for now. Would suggest a slight refactor to avoid calling UpdateComboInformation a second time during processing when not required (also happens in PreProcess), and also switching to foreach to match the style of the implementation in that method closer:

diff --git a/osu.Game/Screens/Edit/EditorBeatmapProcessor.cs b/osu.Game/Screens/Edit/EditorBeatmapProcessor.cs
index 8108f51ad1..33d277c84c 100644
--- a/osu.Game/Screens/Edit/EditorBeatmapProcessor.cs
+++ b/osu.Game/Screens/Edit/EditorBeatmapProcessor.cs
@@ -111,20 +111,30 @@ private void ensureNewComboAfterBreaks()
 
             int currentBreak = 0;
 
-            for (int i = 0; i < Beatmap.HitObjects.Count; ++i)
-            {
-                var hitObject = Beatmap.HitObjects[i];
+            IHasComboInformation? lastObj = null;
+            bool requiredChanges = false;
 
+            foreach (var hitObject in Beatmap.HitObjects)
+            {
                 if (hitObject is not IHasComboInformation hasCombo)
                     continue;
 
-                if (currentBreak < breakEnds.Count && hitObject.StartTime >= breakEnds[currentBreak])
+                if (hitObject.StartTime >= breakEnds[currentBreak])
                 {
-                    hasCombo.NewCombo = true;
-                    currentBreak += 1;
+                    if (!hasCombo.NewCombo)
+                    {
+                        hasCombo.NewCombo = true;
+                        requiredChanges = true;
+                    }
+
+                    if (++currentBreak == breakEnds.Count)
+                        break;
                 }
 
-                hasCombo.UpdateComboInformation(i > 0 ? Beatmap.HitObjects[i - 1] as IHasComboInformation : null);
+                if (requiredChanges)
+                    hasCombo.UpdateComboInformation(lastObj);
+
+                lastObj = hasCombo;
             }
         }
     }

[bdach]

I've made the changes in 7c70dc4, but that's not your patch above applied verbatim. The reason why it's not is that the patch is slightly buggy; the break on ++currentBreak == breakEnds.Count is incorrect because it terminates the entire foreach, which notably includes the "update combo information if necessary" part. Therefore the entire logic would stop working if you inserted an object after the end of the last break in the beatmap.

I've added test coverage in fbfda2e to exercise this because I also stepped on this rake before.