From 7998204cfe1e529bc684b54f64356882ec2c81a2 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Thu, 23 Nov 2023 13:41:01 +0900 Subject: [PATCH] Fix combo/combo colouring issues around spinners --- .../Beatmaps/CatchBeatmapProcessor.cs | 16 ++++++ .../Objects/CatchHitObject.cs | 25 +++++++++ .../Beatmaps/OsuBeatmapProcessor.cs | 54 ++++++++++++------- osu.Game.Rulesets.Osu/Objects/OsuHitObject.cs | 25 +++++++++ .../Formats/LegacyBeatmapDecoderTest.cs | 48 +++++++++++++++++ .../Resources/spinner-between-objects.osu | 38 +++++++++++++ osu.Game/Beatmaps/BeatmapProcessor.cs | 6 --- .../Legacy/Catch/ConvertHitObjectParser.cs | 38 ++++--------- .../Legacy/Osu/ConvertHitObjectParser.cs | 38 ++++--------- 9 files changed, 210 insertions(+), 78 deletions(-) create mode 100644 osu.Game.Tests/Resources/spinner-between-objects.osu diff --git a/osu.Game.Rulesets.Catch/Beatmaps/CatchBeatmapProcessor.cs b/osu.Game.Rulesets.Catch/Beatmaps/CatchBeatmapProcessor.cs index ab61b14ac4..7c81ca03d1 100644 --- a/osu.Game.Rulesets.Catch/Beatmaps/CatchBeatmapProcessor.cs +++ b/osu.Game.Rulesets.Catch/Beatmaps/CatchBeatmapProcessor.cs @@ -23,6 +23,22 @@ namespace osu.Game.Rulesets.Catch.Beatmaps { } + public override void PreProcess() + { + IHasComboInformation? lastObj = null; + + // For sanity, ensures that both the first hitobject and the first hitobject after a banana shower start a new combo. + // This is normally enforced by the legacy decoder, but is not enforced by the editor. + foreach (var obj in Beatmap.HitObjects.OfType()) + { + if (obj is not BananaShower && (lastObj == null || lastObj is BananaShower)) + obj.NewCombo = true; + lastObj = obj; + } + + base.PreProcess(); + } + public override void PostProcess() { base.PostProcess(); diff --git a/osu.Game.Rulesets.Catch/Objects/CatchHitObject.cs b/osu.Game.Rulesets.Catch/Objects/CatchHitObject.cs index b9fef6bf8c..d122758a4e 100644 --- a/osu.Game.Rulesets.Catch/Objects/CatchHitObject.cs +++ b/osu.Game.Rulesets.Catch/Objects/CatchHitObject.cs @@ -155,6 +155,31 @@ namespace osu.Game.Rulesets.Catch.Objects Scale = LegacyRulesetExtensions.CalculateScaleFromCircleSize(difficulty.CircleSize); } + public void UpdateComboInformation(IHasComboInformation? lastObj) + { + ComboIndex = lastObj?.ComboIndex ?? 0; + ComboIndexWithOffsets = lastObj?.ComboIndexWithOffsets ?? 0; + IndexInCurrentCombo = (lastObj?.IndexInCurrentCombo + 1) ?? 0; + + if (this is BananaShower) + { + // For the purpose of combo colours, spinners never start a new combo even if they are flagged as doing so. + return; + } + + // At decode time, the first hitobject in the beatmap and the first hitobject after a banana shower are both enforced to be a new combo, + // but this isn't directly enforced by the editor so the extra checks against the last hitobject are duplicated here. + if (NewCombo || lastObj == null || lastObj is BananaShower) + { + IndexInCurrentCombo = 0; + ComboIndex++; + ComboIndexWithOffsets += ComboOffset + 1; + + if (lastObj != null) + lastObj.LastInCombo = true; + } + } + protected override HitWindows CreateHitWindows() => HitWindows.Empty; #region Hit object conversion diff --git a/osu.Game.Rulesets.Osu/Beatmaps/OsuBeatmapProcessor.cs b/osu.Game.Rulesets.Osu/Beatmaps/OsuBeatmapProcessor.cs index c081df3ac6..835c67ff19 100644 --- a/osu.Game.Rulesets.Osu/Beatmaps/OsuBeatmapProcessor.cs +++ b/osu.Game.Rulesets.Osu/Beatmaps/OsuBeatmapProcessor.cs @@ -2,9 +2,11 @@ // See the LICENCE file in the repository root for full licence text. using System; +using System.Linq; using osu.Framework.Graphics; using osu.Game.Beatmaps; using osu.Game.Rulesets.Objects; +using osu.Game.Rulesets.Objects.Types; using osu.Game.Rulesets.Osu.Objects; using osuTK; @@ -19,6 +21,22 @@ namespace osu.Game.Rulesets.Osu.Beatmaps { } + public override void PreProcess() + { + IHasComboInformation? lastObj = null; + + // For sanity, ensures that both the first hitobject and the first hitobject after a spinner start a new combo. + // This is normally enforced by the legacy decoder, but is not enforced by the editor. + foreach (var obj in Beatmap.HitObjects.OfType()) + { + if (obj is not Spinner && (lastObj == null || lastObj is Spinner)) + obj.NewCombo = true; + lastObj = obj; + } + + base.PreProcess(); + } + public override void PostProcess() { base.PostProcess(); @@ -95,15 +113,15 @@ namespace osu.Game.Rulesets.Osu.Beatmaps { int n = i; /* We should check every note which has not yet got a stack. - * Consider the case we have two interwound stacks and this will make sense. - * - * o <-1 o <-2 - * o <-3 o <-4 - * - * We first process starting from 4 and handle 2, - * then we come backwards on the i loop iteration until we reach 3 and handle 1. - * 2 and 1 will be ignored in the i loop because they already have a stack value. - */ + * Consider the case we have two interwound stacks and this will make sense. + * + * o <-1 o <-2 + * o <-3 o <-4 + * + * We first process starting from 4 and handle 2, + * then we come backwards on the i loop iteration until we reach 3 and handle 1. + * 2 and 1 will be ignored in the i loop because they already have a stack value. + */ OsuHitObject objectI = beatmap.HitObjects[i]; if (objectI.StackHeight != 0 || objectI is Spinner) continue; @@ -111,9 +129,9 @@ namespace osu.Game.Rulesets.Osu.Beatmaps double stackThreshold = objectI.TimePreempt * beatmap.BeatmapInfo.StackLeniency; /* If this object is a hitcircle, then we enter this "special" case. - * It either ends with a stack of hitcircles only, or a stack of hitcircles that are underneath a slider. - * Any other case is handled by the "is Slider" code below this. - */ + * It either ends with a stack of hitcircles only, or a stack of hitcircles that are underneath a slider. + * Any other case is handled by the "is Slider" code below this. + */ if (objectI is HitCircle) { while (--n >= 0) @@ -135,10 +153,10 @@ namespace osu.Game.Rulesets.Osu.Beatmaps } /* This is a special case where hticircles are moved DOWN and RIGHT (negative stacking) if they are under the *last* slider in a stacked pattern. - * o==o <- slider is at original location - * o <- hitCircle has stack of -1 - * o <- hitCircle has stack of -2 - */ + * o==o <- slider is at original location + * o <- hitCircle has stack of -1 + * o <- hitCircle has stack of -2 + */ if (objectN is Slider && Vector2Extensions.Distance(objectN.EndPosition, objectI.Position) < stack_distance) { int offset = objectI.StackHeight - objectN.StackHeight + 1; @@ -169,8 +187,8 @@ namespace osu.Game.Rulesets.Osu.Beatmaps else if (objectI is Slider) { /* We have hit the first slider in a possible stack. - * From this point on, we ALWAYS stack positive regardless. - */ + * From this point on, we ALWAYS stack positive regardless. + */ while (--n >= startIndex) { OsuHitObject objectN = beatmap.HitObjects[n]; diff --git a/osu.Game.Rulesets.Osu/Objects/OsuHitObject.cs b/osu.Game.Rulesets.Osu/Objects/OsuHitObject.cs index d74d28c748..716c34d024 100644 --- a/osu.Game.Rulesets.Osu/Objects/OsuHitObject.cs +++ b/osu.Game.Rulesets.Osu/Objects/OsuHitObject.cs @@ -159,6 +159,31 @@ namespace osu.Game.Rulesets.Osu.Objects Scale = LegacyRulesetExtensions.CalculateScaleFromCircleSize(difficulty.CircleSize, true); } + public void UpdateComboInformation(IHasComboInformation? lastObj) + { + ComboIndex = lastObj?.ComboIndex ?? 0; + ComboIndexWithOffsets = lastObj?.ComboIndexWithOffsets ?? 0; + IndexInCurrentCombo = (lastObj?.IndexInCurrentCombo + 1) ?? 0; + + if (this is Spinner) + { + // For the purpose of combo colours, spinners never start a new combo even if they are flagged as doing so. + return; + } + + // At decode time, the first hitobject in the beatmap and the first hitobject after a spinner are both enforced to be a new combo, + // but this isn't directly enforced by the editor so the extra checks against the last hitobject are duplicated here. + if (NewCombo || lastObj == null || lastObj is Spinner) + { + IndexInCurrentCombo = 0; + ComboIndex++; + ComboIndexWithOffsets += ComboOffset + 1; + + if (lastObj != null) + lastObj.LastInCombo = true; + } + } + protected override HitWindows CreateHitWindows() => new OsuHitWindows(); } } diff --git a/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs b/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs index be1993957f..20f59384ab 100644 --- a/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs +++ b/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs @@ -12,6 +12,7 @@ using osu.Game.Beatmaps.Formats; using osu.Game.Beatmaps.Legacy; using osu.Game.Beatmaps.Timing; using osu.Game.IO; +using osu.Game.Rulesets; using osu.Game.Rulesets.Catch; using osu.Game.Rulesets.Catch.Beatmaps; using osu.Game.Rulesets.Mods; @@ -1108,5 +1109,52 @@ namespace osu.Game.Tests.Beatmaps.Formats Assert.That(((IHasCombo)beatmap.HitObjects[2]).NewCombo, Is.False); } } + + /// + /// Test cases that involve a spinner between two hitobjects. + /// + [Test] + public void TestSpinnerNewComboBetweenObjects([Values("osu", "catch")] string rulesetName) + { + var decoder = new LegacyBeatmapDecoder { ApplyOffsets = false }; + + using (var resStream = TestResources.OpenResource("spinner-between-objects.osu")) + using (var stream = new LineBufferedReader(resStream)) + { + Ruleset ruleset; + + switch (rulesetName) + { + case "osu": + ruleset = new OsuRuleset(); + break; + + case "catch": + ruleset = new CatchRuleset(); + break; + + default: + throw new ArgumentOutOfRangeException(nameof(rulesetName), rulesetName, null); + } + + var working = new TestWorkingBeatmap(decoder.Decode(stream)); + var playable = working.GetPlayableBeatmap(ruleset.RulesetInfo, Array.Empty()); + + // There's no good way to figure out these values other than to compare (in code) with osu!stable... + + Assert.That(((IHasComboInformation)playable.HitObjects[0]).ComboIndexWithOffsets, Is.EqualTo(1)); + Assert.That(((IHasComboInformation)playable.HitObjects[2]).ComboIndexWithOffsets, Is.EqualTo(2)); + Assert.That(((IHasComboInformation)playable.HitObjects[3]).ComboIndexWithOffsets, Is.EqualTo(2)); + Assert.That(((IHasComboInformation)playable.HitObjects[5]).ComboIndexWithOffsets, Is.EqualTo(3)); + Assert.That(((IHasComboInformation)playable.HitObjects[6]).ComboIndexWithOffsets, Is.EqualTo(3)); + Assert.That(((IHasComboInformation)playable.HitObjects[8]).ComboIndexWithOffsets, Is.EqualTo(4)); + Assert.That(((IHasComboInformation)playable.HitObjects[9]).ComboIndexWithOffsets, Is.EqualTo(4)); + Assert.That(((IHasComboInformation)playable.HitObjects[11]).ComboIndexWithOffsets, Is.EqualTo(5)); + Assert.That(((IHasComboInformation)playable.HitObjects[12]).ComboIndexWithOffsets, Is.EqualTo(6)); + Assert.That(((IHasComboInformation)playable.HitObjects[14]).ComboIndexWithOffsets, Is.EqualTo(7)); + Assert.That(((IHasComboInformation)playable.HitObjects[15]).ComboIndexWithOffsets, Is.EqualTo(8)); + Assert.That(((IHasComboInformation)playable.HitObjects[17]).ComboIndexWithOffsets, Is.EqualTo(9)); + } + } } } diff --git a/osu.Game.Tests/Resources/spinner-between-objects.osu b/osu.Game.Tests/Resources/spinner-between-objects.osu new file mode 100644 index 0000000000..03e61d965c --- /dev/null +++ b/osu.Game.Tests/Resources/spinner-between-objects.osu @@ -0,0 +1,38 @@ +osu file format v14 + +[General] +Mode: 0 + +[TimingPoints] +0,571.428571428571,4,2,1,5,1,0 + +[HitObjects] +// +C -> +C -> +C +104,95,0,5,0,0:0:0:0: +256,192,1000,12,0,2000,0:0:0:0: +178,171,3000,5,0,0:0:0:0: + +// -C -> +C -> +C +178,171,4000,1,0,0:0:0:0: +256,192,5000,12,0,6000,0:0:0:0: +178,171,7000,5,0,0:0:0:0: + +// -C -> -C -> +C +178,171,8000,1,0,0:0:0:0: +256,192,9000,8,0,10000,0:0:0:0: +178,171,11000,5,0,0:0:0:0: + +// -C -> -C -> -C +178,171,12000,1,0,0:0:0:0: +256,192,13000,8,0,14000,0:0:0:0: +178,171,15000,1,0,0:0:0:0: + +// +C -> -C -> -C +178,171,16000,5,0,0:0:0:0: +256,192,17000,8,0,18000,0:0:0:0: +178,171,19000,1,0,0:0:0:0: + +// +C -> +C -> -C +178,171,20000,5,0,0:0:0:0: +256,192,21000,12,0,22000,0:0:0:0: +178,171,23000,1,0,0:0:0:0: \ No newline at end of file diff --git a/osu.Game/Beatmaps/BeatmapProcessor.cs b/osu.Game/Beatmaps/BeatmapProcessor.cs index fb5313469f..89d6e9d3f8 100644 --- a/osu.Game/Beatmaps/BeatmapProcessor.cs +++ b/osu.Game/Beatmaps/BeatmapProcessor.cs @@ -24,12 +24,6 @@ namespace osu.Game.Beatmaps foreach (var obj in Beatmap.HitObjects.OfType()) { - if (lastObj == null) - { - // first hitobject should always be marked as a new combo for sanity. - obj.NewCombo = true; - } - obj.UpdateComboInformation(lastObj); lastObj = obj; } diff --git a/osu.Game/Rulesets/Objects/Legacy/Catch/ConvertHitObjectParser.cs b/osu.Game/Rulesets/Objects/Legacy/Catch/ConvertHitObjectParser.cs index 4861e8b3f7..0ed5aef0cf 100644 --- a/osu.Game/Rulesets/Objects/Legacy/Catch/ConvertHitObjectParser.cs +++ b/osu.Game/Rulesets/Objects/Legacy/Catch/ConvertHitObjectParser.cs @@ -14,26 +14,19 @@ namespace osu.Game.Rulesets.Objects.Legacy.Catch /// public class ConvertHitObjectParser : Legacy.ConvertHitObjectParser { + private ConvertHitObject lastObject; + public ConvertHitObjectParser(double offset, int formatVersion) : base(offset, formatVersion) { } - private bool forceNewCombo; - private int extraComboOffset; - protected override HitObject CreateHit(Vector2 position, bool newCombo, int comboOffset) { - newCombo |= forceNewCombo; - comboOffset += extraComboOffset; - - forceNewCombo = false; - extraComboOffset = 0; - - return new ConvertHit + return lastObject = new ConvertHit { Position = position, - NewCombo = newCombo, + NewCombo = FirstObject || lastObject is ConvertSpinner || newCombo, ComboOffset = comboOffset }; } @@ -41,16 +34,10 @@ namespace osu.Game.Rulesets.Objects.Legacy.Catch protected override HitObject CreateSlider(Vector2 position, bool newCombo, int comboOffset, PathControlPoint[] controlPoints, double? length, int repeatCount, IList> nodeSamples) { - newCombo |= forceNewCombo; - comboOffset += extraComboOffset; - - forceNewCombo = false; - extraComboOffset = 0; - - return new ConvertSlider + return lastObject = new ConvertSlider { Position = position, - NewCombo = FirstObject || newCombo, + NewCombo = FirstObject || lastObject is ConvertSpinner || newCombo, ComboOffset = comboOffset, Path = new SliderPath(controlPoints, length), NodeSamples = nodeSamples, @@ -60,20 +47,17 @@ namespace osu.Game.Rulesets.Objects.Legacy.Catch protected override HitObject CreateSpinner(Vector2 position, bool newCombo, int comboOffset, double duration) { - // Convert spinners don't create the new combo themselves, but force the next non-spinner hitobject to create a new combo - // Their combo offset is still added to that next hitobject's combo index - forceNewCombo |= FormatVersion <= 8 || newCombo; - extraComboOffset += comboOffset; - - return new ConvertSpinner + return lastObject = new ConvertSpinner { - Duration = duration + Duration = duration, + NewCombo = newCombo + // Spinners cannot have combo offset. }; } protected override HitObject CreateHold(Vector2 position, bool newCombo, int comboOffset, double duration) { - return null; + return lastObject = null; } } } diff --git a/osu.Game/Rulesets/Objects/Legacy/Osu/ConvertHitObjectParser.cs b/osu.Game/Rulesets/Objects/Legacy/Osu/ConvertHitObjectParser.cs index 7a88a31bd5..8bb9600a7d 100644 --- a/osu.Game/Rulesets/Objects/Legacy/Osu/ConvertHitObjectParser.cs +++ b/osu.Game/Rulesets/Objects/Legacy/Osu/ConvertHitObjectParser.cs @@ -14,26 +14,19 @@ namespace osu.Game.Rulesets.Objects.Legacy.Osu /// public class ConvertHitObjectParser : Legacy.ConvertHitObjectParser { + private ConvertHitObject lastObject; + public ConvertHitObjectParser(double offset, int formatVersion) : base(offset, formatVersion) { } - private bool forceNewCombo; - private int extraComboOffset; - protected override HitObject CreateHit(Vector2 position, bool newCombo, int comboOffset) { - newCombo |= forceNewCombo; - comboOffset += extraComboOffset; - - forceNewCombo = false; - extraComboOffset = 0; - - return new ConvertHit + return lastObject = new ConvertHit { Position = position, - NewCombo = FirstObject || newCombo, + NewCombo = FirstObject || lastObject is ConvertSpinner || newCombo, ComboOffset = comboOffset }; } @@ -41,16 +34,10 @@ namespace osu.Game.Rulesets.Objects.Legacy.Osu protected override HitObject CreateSlider(Vector2 position, bool newCombo, int comboOffset, PathControlPoint[] controlPoints, double? length, int repeatCount, IList> nodeSamples) { - newCombo |= forceNewCombo; - comboOffset += extraComboOffset; - - forceNewCombo = false; - extraComboOffset = 0; - - return new ConvertSlider + return lastObject = new ConvertSlider { Position = position, - NewCombo = FirstObject || newCombo, + NewCombo = FirstObject || lastObject is ConvertSpinner || newCombo, ComboOffset = comboOffset, Path = new SliderPath(controlPoints, length), NodeSamples = nodeSamples, @@ -60,21 +47,18 @@ namespace osu.Game.Rulesets.Objects.Legacy.Osu protected override HitObject CreateSpinner(Vector2 position, bool newCombo, int comboOffset, double duration) { - // Convert spinners don't create the new combo themselves, but force the next non-spinner hitobject to create a new combo - // Their combo offset is still added to that next hitobject's combo index - forceNewCombo |= FormatVersion <= 8 || newCombo; - extraComboOffset += comboOffset; - - return new ConvertSpinner + return lastObject = new ConvertSpinner { Position = position, - Duration = duration + Duration = duration, + NewCombo = newCombo + // Spinners cannot have combo offset. }; } protected override HitObject CreateHold(Vector2 position, bool newCombo, int comboOffset, double duration) { - return null; + return lastObject = null; } } }