Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Force new combo on objects succeeding a break #31448

Merged
merged 4 commits into from
Jan 8, 2025
Merged

Force new combo on objects succeeding a break #31448

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
973f606
Add test coverage for expected behaviour
bdach Jan 7, 2025
c93b875
Force new combo on objects succeeding a break
bdach Jan 7, 2025
fbfda2e
Extend test coverage with combo index correctness checks
bdach Jan 8, 2025
7c70dc4
Only update combo information when any changes happened
bdach Jan 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 80 additions & 0 deletions osu.Game.Tests/Editing/TestSceneEditorBeatmapProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,5 +539,85 @@ public void TestTimePreemptIsRespected()
Assert.That(beatmap.Breaks[0].EndTime, Is.EqualTo(5000 - OsuHitObject.PREEMPT_MAX));
});
}

[Test]
public void TestPuttingObjectBetweenBreakEndAndAnotherObjectForcesNewCombo()
{
var controlPoints = new ControlPointInfo();
controlPoints.Add(0, new TimingControlPoint { BeatLength = 500 });
var beatmap = new EditorBeatmap(new Beatmap
{
ControlPointInfo = controlPoints,
BeatmapInfo = { Ruleset = new OsuRuleset().RulesetInfo },
Difficulty =
{
ApproachRate = 10,
},
HitObjects =
{
new HitCircle { StartTime = 1000, NewCombo = true },
new HitCircle { StartTime = 4500 },
new HitCircle { StartTime = 5000, NewCombo = true },
},
Breaks =
{
new BreakPeriod(2000, 4000),
}
});

foreach (var ho in beatmap.HitObjects)
ho.ApplyDefaults(beatmap.ControlPointInfo, beatmap.Difficulty);

var beatmapProcessor = new EditorBeatmapProcessor(beatmap, new OsuRuleset());
beatmapProcessor.PreProcess();
beatmapProcessor.PostProcess();

Assert.Multiple(() =>
{
Assert.That(((HitCircle)beatmap.HitObjects[1]).NewCombo, Is.True);
Assert.That(((HitCircle)beatmap.HitObjects[2]).NewCombo, Is.True);

Assert.That(((HitCircle)beatmap.HitObjects[0]).ComboIndex, Is.EqualTo(1));
Assert.That(((HitCircle)beatmap.HitObjects[1]).ComboIndex, Is.EqualTo(2));
Assert.That(((HitCircle)beatmap.HitObjects[2]).ComboIndex, Is.EqualTo(3));
});
}

[Test]
public void TestAutomaticallyInsertedBreakForcesNewCombo()
{
var controlPoints = new ControlPointInfo();
controlPoints.Add(0, new TimingControlPoint { BeatLength = 500 });
var beatmap = new EditorBeatmap(new Beatmap
{
ControlPointInfo = controlPoints,
BeatmapInfo = { Ruleset = new OsuRuleset().RulesetInfo },
Difficulty =
{
ApproachRate = 10,
},
HitObjects =
{
new HitCircle { StartTime = 1000, NewCombo = true },
new HitCircle { StartTime = 5000 },
},
});

foreach (var ho in beatmap.HitObjects)
ho.ApplyDefaults(beatmap.ControlPointInfo, beatmap.Difficulty);

var beatmapProcessor = new EditorBeatmapProcessor(beatmap, new OsuRuleset());
beatmapProcessor.PreProcess();
beatmapProcessor.PostProcess();

Assert.Multiple(() =>
{
Assert.That(beatmap.Breaks, Has.Count.EqualTo(1));
Assert.That(((HitCircle)beatmap.HitObjects[1]).NewCombo, Is.True);

Assert.That(((HitCircle)beatmap.HitObjects[0]).ComboIndex, Is.EqualTo(1));
Assert.That(((HitCircle)beatmap.HitObjects[1]).ComboIndex, Is.EqualTo(2));
});
}
}
}
36 changes: 36 additions & 0 deletions osu.Game/Screens/Edit/EditorBeatmapProcessor.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ public void PostProcess()
rulesetBeatmapProcessor?.PostProcess();

autoGenerateBreaks();
ensureNewComboAfterBreaks();
}

private void autoGenerateBreaks()
Expand Down Expand Up @@ -100,5 +101,40 @@ private void autoGenerateBreaks()
Beatmap.Breaks.Add(breakPeriod);
}
}

private void ensureNewComboAfterBreaks()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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;
             }
         }
     }

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
var breakEnds = Beatmap.Breaks.Select(b => b.EndTime).OrderBy(t => t).ToList();

if (breakEnds.Count == 0)
return;

int currentBreak = 0;

IHasComboInformation? lastObj = null;
bool comboInformationUpdateRequired = false;

foreach (var hitObject in Beatmap.HitObjects)
{
if (hitObject is not IHasComboInformation hasCombo)
continue;

if (currentBreak < breakEnds.Count && hitObject.StartTime >= breakEnds[currentBreak])
{
if (!hasCombo.NewCombo)
{
hasCombo.NewCombo = true;
comboInformationUpdateRequired = true;
}

currentBreak += 1;
}

if (comboInformationUpdateRequired)
hasCombo.UpdateComboInformation(lastObj);

lastObj = hasCombo;
}
}
}
}
Loading