Merge pull request #8818 from smoogipoo/fix-beatmap-encoder

Fix beatmap encoder by enabling untested test cases

osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapEncoderTest.cs

@@ -1,14 +1,23 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System;
+using System.Collections;
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
 using NUnit.Framework;
+using osu.Framework.Audio.Track;
+using osu.Framework.Graphics.Textures;
 using osu.Game.Beatmaps;
+using osu.Game.Beatmaps.ControlPoints;
 using osu.Game.Beatmaps.Formats;
 using osu.Game.IO;
 using osu.Game.IO.Serialization;
+using osu.Game.Rulesets.Catch;
+using osu.Game.Rulesets.Mania;
+using osu.Game.Rulesets.Osu;
+using osu.Game.Rulesets.Taiko;
 using osu.Game.Tests.Resources;
 
 namespace osu.Game.Tests.Beatmaps.Formats
@@ -16,39 +25,91 @@ namespace osu.Game.Tests.Beatmaps.Formats
     [TestFixture]
     public class LegacyBeatmapEncoderTest
     {
-        private const string normal = "Soleily - Renatus (Gamu) [Insane].osu";
-
         private static IEnumerable<string> allBeatmaps => TestResources.GetStore().GetAvailableResources().Where(res => res.EndsWith(".osu"));
 
         [TestCaseSource(nameof(allBeatmaps))]
-        public void TestDecodeEncodedBeatmap(string name)
+        public void TestBeatmap(string name)
         {
-            var decoded = decode(normal, out var encoded);
+            var decoded = decode(name, out var encoded);
+
+            sort(decoded);
+            sort(encoded);
 
-            Assert.That(decoded.HitObjects.Count, Is.EqualTo(encoded.HitObjects.Count));
             Assert.That(encoded.Serialize(), Is.EqualTo(decoded.Serialize()));
         }
 
-        private Beatmap decode(string filename, out Beatmap encoded)
+        private void sort(IBeatmap beatmap)
         {
-            using (var stream = TestResources.OpenResource(filename))
+            // Sort control points to ensure a sane ordering, as they may be parsed in different orders. This works because each group contains only uniquely-typed control points.
+            foreach (var g in beatmap.ControlPointInfo.Groups)
+            {
+                ArrayList.Adapter((IList)g.ControlPoints).Sort(
+                    Comparer<ControlPoint>.Create((c1, c2) => string.Compare(c1.GetType().ToString(), c2.GetType().ToString(), StringComparison.Ordinal)));
+            }
+        }
+
+        private IBeatmap decode(string filename, out IBeatmap encoded)
+        {
+            using (var stream = TestResources.GetStore().GetStream(filename))
             using (var sr = new LineBufferedReader(stream))
             {
-                var legacyDecoded = new LegacyBeatmapDecoder { ApplyOffsets = false }.Decode(sr);
+                var legacyDecoded = convert(new LegacyBeatmapDecoder { ApplyOffsets = false }.Decode(sr));
 
                 using (var ms = new MemoryStream())
                 using (var sw = new StreamWriter(ms))
-                using (var sr2 = new LineBufferedReader(ms))
+                using (var sr2 = new LineBufferedReader(ms, true))
                 {
                     new LegacyBeatmapEncoder(legacyDecoded).Encode(sw);
-                    sw.Flush();
 
+                    sw.Flush();
                     ms.Position = 0;
 
-                    encoded = new LegacyBeatmapDecoder { ApplyOffsets = false }.Decode(sr2);
+                    encoded = convert(new LegacyBeatmapDecoder { ApplyOffsets = false }.Decode(sr2));
+
                     return legacyDecoded;
                 }
             }
         }
+
+        private IBeatmap convert(IBeatmap beatmap)
+        {
+            switch (beatmap.BeatmapInfo.RulesetID)
+            {
+                case 0:
+                    beatmap.BeatmapInfo.Ruleset = new OsuRuleset().RulesetInfo;
+                    break;
+
+                case 1:
+                    beatmap.BeatmapInfo.Ruleset = new TaikoRuleset().RulesetInfo;
+                    break;
+
+                case 2:
+                    beatmap.BeatmapInfo.Ruleset = new CatchRuleset().RulesetInfo;
+                    break;
+
+                case 3:
+                    beatmap.BeatmapInfo.Ruleset = new ManiaRuleset().RulesetInfo;
+                    break;
+            }
+
+            return new TestWorkingBeatmap(beatmap).GetPlayableBeatmap(beatmap.BeatmapInfo.Ruleset);
+        }
+
+        private class TestWorkingBeatmap : WorkingBeatmap
+        {
+            private readonly IBeatmap beatmap;
+
+            public TestWorkingBeatmap(IBeatmap beatmap)
+                : base(beatmap.BeatmapInfo, null)
+            {
+                this.beatmap = beatmap;
+            }
+
+            protected override IBeatmap GetBeatmap() => beatmap;
+
+            protected override Texture GetBackground() => throw new NotImplementedException();
+
+            protected override Track GetTrack() => throw new NotImplementedException();
+        }
     }
 }

osu.Game.Tests/Resources/hitobject-combo-offset.osu

@@ -5,27 +5,27 @@ osu file format v14
 255,193,1000,49,0,0:0:0:0:
 // Combo index = 4
 
-// Slider with new combo followed by circle with no new combo
+// Spinner with new combo followed by circle with no new combo
 256,192,2000,12,0,2000,0:0:0:0:
 255,193,3000,1,0,0:0:0:0:
 // Combo index = 5
 
-// Slider without new combo followed by circle with no new combo
+// Spinner without new combo followed by circle with no new combo
 256,192,4000,8,0,5000,0:0:0:0:
 255,193,6000,1,0,0:0:0:0:
 // Combo index = 5
 
-// Slider without new combo followed by circle with new combo
+// Spinner without new combo followed by circle with new combo
 256,192,7000,8,0,8000,0:0:0:0:
 255,193,9000,5,0,0:0:0:0:
 // Combo index = 6
 
-// Slider with new combo and offset (1) followed by circle with new combo and offset (3)
+// Spinner with new combo and offset (1) followed by circle with new combo and offset (3)
 256,192,10000,28,0,11000,0:0:0:0:
 255,193,12000,53,0,0:0:0:0:
 // Combo index = 11
 
-// Slider with new combo and offset (2) followed by slider with no new combo followed by circle with no new combo
+// Spinner with new combo and offset (2) followed by slider with no new combo followed by circle with no new combo
 256,192,13000,44,0,14000,0:0:0:0:
 256,192,15000,8,0,16000,0:0:0:0:
 255,193,17000,1,0,0:0:0:0:

osu.Game.Tests/Resources/sample-beatmap-osu.osu

@@ -0,0 +1,32 @@
+osu file format v14
+
+[General]
+SampleSet: Normal
+StackLeniency: 0.7
+Mode: 0
+
+[Difficulty]
+HPDrainRate:3
+CircleSize:5
+OverallDifficulty:8
+ApproachRate:8
+SliderMultiplier:3.59999990463257
+SliderTickRate:2
+
+[TimingPoints]
+24,352.941176470588,4,1,1,100,1,0
+6376,-50,4,1,1,100,0,0
+
+[HitObjects]
+98,69,24,1,0,0:0:0:0:
+419,72,200,1,2,0:0:0:0:
+81,314,376,1,6,0:0:0:0:
+423,321,553,1,12,0:0:0:0:
+86,192,729,2,0,P|459:193|460:193,1,359.999990463257
+86,192,1259,2,0,P|246:82|453:203,1,449.999988079071
+86,192,1876,2,0,B|256:30|257:313|464:177,1,359.999990463257
+86,55,2406,2,12,B|447:51|447:51|452:348|452:348|78:344,1,989.999973773957,14|2,0:0|0:0,0:0:0:0:
+256,192,3553,12,0,4259,0:0:0:0:
+67,57,4435,5,0,0:0:0:0:
+440,52,4612,5,0,0:0:0:0:
+86,181,4788,6,0,L|492:183,1,359.999990463257

osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs

@@ -3,6 +3,7 @@
 
 using System;
 using System.Collections.Generic;
+using System.Globalization;
 using System.IO;
 using System.Linq;
 using System.Text;
@@ -10,6 +11,7 @@ using osu.Game.Audio;
 using osu.Game.Beatmaps.ControlPoints;
 using osu.Game.Beatmaps.Legacy;
 using osu.Game.Rulesets.Objects;
+using osu.Game.Rulesets.Objects.Legacy;
 using osu.Game.Rulesets.Objects.Types;
 
 namespace osu.Game.Beatmaps.Formats
@@ -48,7 +50,7 @@ namespace osu.Game.Beatmaps.Formats
             handleEvents(writer);
 
             writer.WriteLine();
-            handleTimingPoints(writer);
+            handleControlPoints(writer);
 
             writer.WriteLine();
             handleHitObjects(writer);
@@ -58,7 +60,7 @@ namespace osu.Game.Beatmaps.Formats
         {
             writer.WriteLine("[General]");
 
-            writer.WriteLine(FormattableString.Invariant($"AudioFilename: {Path.GetFileName(beatmap.Metadata.AudioFile)}"));
+            if (beatmap.Metadata.AudioFile != null) writer.WriteLine(FormattableString.Invariant($"AudioFilename: {Path.GetFileName(beatmap.Metadata.AudioFile)}"));
             writer.WriteLine(FormattableString.Invariant($"AudioLeadIn: {beatmap.BeatmapInfo.AudioLeadIn}"));
             writer.WriteLine(FormattableString.Invariant($"PreviewTime: {beatmap.Metadata.PreviewTime}"));
             // Todo: Not all countdown types are supported by lazer yet
@@ -103,15 +105,15 @@ namespace osu.Game.Beatmaps.Formats
             writer.WriteLine("[Metadata]");
 
             writer.WriteLine(FormattableString.Invariant($"Title: {beatmap.Metadata.Title}"));
-            writer.WriteLine(FormattableString.Invariant($"TitleUnicode: {beatmap.Metadata.TitleUnicode}"));
+            if (beatmap.Metadata.TitleUnicode != null) writer.WriteLine(FormattableString.Invariant($"TitleUnicode: {beatmap.Metadata.TitleUnicode}"));
             writer.WriteLine(FormattableString.Invariant($"Artist: {beatmap.Metadata.Artist}"));
-            writer.WriteLine(FormattableString.Invariant($"ArtistUnicode: {beatmap.Metadata.ArtistUnicode}"));
+            if (beatmap.Metadata.ArtistUnicode != null) writer.WriteLine(FormattableString.Invariant($"ArtistUnicode: {beatmap.Metadata.ArtistUnicode}"));
             writer.WriteLine(FormattableString.Invariant($"Creator: {beatmap.Metadata.AuthorString}"));
             writer.WriteLine(FormattableString.Invariant($"Version: {beatmap.BeatmapInfo.Version}"));
-            writer.WriteLine(FormattableString.Invariant($"Source: {beatmap.Metadata.Source}"));
+            if (beatmap.Metadata.Source != null) writer.WriteLine(FormattableString.Invariant($"Source: {beatmap.Metadata.Source}"));
-            writer.WriteLine(FormattableString.Invariant($"Tags: {beatmap.Metadata.Tags}"));
+            if (beatmap.Metadata.Tags != null) writer.WriteLine(FormattableString.Invariant($"Tags: {beatmap.Metadata.Tags}"));
-            writer.WriteLine(FormattableString.Invariant($"BeatmapID: {beatmap.BeatmapInfo.OnlineBeatmapID ?? 0}"));
+            if (beatmap.BeatmapInfo.OnlineBeatmapID != null) writer.WriteLine(FormattableString.Invariant($"BeatmapID: {beatmap.BeatmapInfo.OnlineBeatmapID}"));
-            writer.WriteLine(FormattableString.Invariant($"BeatmapSetID: {beatmap.BeatmapInfo.BeatmapSet?.OnlineBeatmapSetID ?? -1}"));
+            if (beatmap.BeatmapInfo.BeatmapSet?.OnlineBeatmapSetID != null) writer.WriteLine(FormattableString.Invariant($"BeatmapSetID: {beatmap.BeatmapInfo.BeatmapSet.OnlineBeatmapSetID}"));
         }
 
         private void handleDifficulty(TextWriter writer)
@@ -137,7 +139,7 @@ namespace osu.Game.Beatmaps.Formats
                 writer.WriteLine(FormattableString.Invariant($"{(int)LegacyEventType.Break},{b.StartTime},{b.EndTime}"));
         }
 
-        private void handleTimingPoints(TextWriter writer)
+        private void handleControlPoints(TextWriter writer)
         {
             if (beatmap.ControlPointInfo.Groups.Count == 0)
                 return;
@@ -146,20 +148,30 @@ namespace osu.Game.Beatmaps.Formats
 
             foreach (var group in beatmap.ControlPointInfo.Groups)
             {
-                var timingPoint = group.ControlPoints.OfType<TimingControlPoint>().FirstOrDefault();
+                var groupTimingPoint = group.ControlPoints.OfType<TimingControlPoint>().FirstOrDefault();
-                var difficultyPoint = beatmap.ControlPointInfo.DifficultyPointAt(group.Time);
-                var samplePoint = beatmap.ControlPointInfo.SamplePointAt(group.Time);
-                var effectPoint = beatmap.ControlPointInfo.EffectPointAt(group.Time);
 
-                // Convert beat length the legacy format
+                // If the group contains a timing control point, it needs to be output separately.
-                double beatLength;
+                if (groupTimingPoint != null)
-                if (timingPoint != null)
+                {
-                    beatLength = timingPoint.BeatLength;
+                    writer.Write(FormattableString.Invariant($"{groupTimingPoint.Time},"));
-                else
+                    writer.Write(FormattableString.Invariant($"{groupTimingPoint.BeatLength},"));
-                    beatLength = -100 / difficultyPoint.SpeedMultiplier;
+                    outputControlPointEffectsAt(groupTimingPoint.Time, true);
+                }
+
+                // Output any remaining effects as secondary non-timing control point.
+                var difficultyPoint = beatmap.ControlPointInfo.DifficultyPointAt(group.Time);
+                writer.Write(FormattableString.Invariant($"{group.Time},"));
+                writer.Write(FormattableString.Invariant($"{-100 / difficultyPoint.SpeedMultiplier},"));
+                outputControlPointEffectsAt(group.Time, false);
+            }
+
+            void outputControlPointEffectsAt(double time, bool isTimingPoint)
+            {
+                var samplePoint = beatmap.ControlPointInfo.SamplePointAt(time);
+                var effectPoint = beatmap.ControlPointInfo.EffectPointAt(time);
 
                 // Apply the control point to a hit sample to uncover legacy properties (e.g. suffix)
-                HitSampleInfo tempHitSample = samplePoint.ApplyTo(new HitSampleInfo());
+                HitSampleInfo tempHitSample = samplePoint.ApplyTo(new ConvertHitObjectParser.LegacyHitSampleInfo());
 
                 // Convert effect flags to the legacy format
                 LegacyEffectFlags effectFlags = LegacyEffectFlags.None;
@@ -168,13 +180,11 @@ namespace osu.Game.Beatmaps.Formats
                 if (effectPoint.OmitFirstBarLine)
                     effectFlags |= LegacyEffectFlags.OmitFirstBarLine;
 
-                writer.Write(FormattableString.Invariant($"{group.Time},"));
+                writer.Write(FormattableString.Invariant($"{(int)beatmap.ControlPointInfo.TimingPointAt(time).TimeSignature},"));
-                writer.Write(FormattableString.Invariant($"{beatLength},"));
-                writer.Write(FormattableString.Invariant($"{(int)beatmap.ControlPointInfo.TimingPointAt(group.Time).TimeSignature},"));
                 writer.Write(FormattableString.Invariant($"{(int)toLegacySampleBank(tempHitSample.Bank)},"));
-                writer.Write(FormattableString.Invariant($"{toLegacyCustomSampleBank(tempHitSample.Suffix)},"));
+                writer.Write(FormattableString.Invariant($"{toLegacyCustomSampleBank(tempHitSample)},"));
                 writer.Write(FormattableString.Invariant($"{tempHitSample.Volume},"));
-                writer.Write(FormattableString.Invariant($"{(timingPoint != null ? '1' : '0')},"));
+                writer.Write(FormattableString.Invariant($"{(isTimingPoint ? '1' : '0')},"));
                 writer.Write(FormattableString.Invariant($"{(int)effectFlags}"));
                 writer.WriteLine();
             }
@@ -240,7 +250,7 @@ namespace osu.Game.Beatmaps.Formats
                     break;
 
                 case IHasEndTime _:
-                    type |= LegacyHitObjectType.Spinner | LegacyHitObjectType.NewCombo;
+                    type |= LegacyHitObjectType.Spinner;
                     break;
 
                 default:
@@ -326,7 +336,7 @@ namespace osu.Game.Beatmaps.Formats
 
             if (!banksOnly)
             {
-                string customSampleBank = toLegacyCustomSampleBank(samples.FirstOrDefault(s => !string.IsNullOrEmpty(s.Name))?.Suffix);
+                string customSampleBank = toLegacyCustomSampleBank(samples.FirstOrDefault(s => !string.IsNullOrEmpty(s.Name)));
                 string sampleFilename = samples.FirstOrDefault(s => string.IsNullOrEmpty(s.Name))?.LookupNames.First() ?? string.Empty;
                 int volume = samples.FirstOrDefault()?.Volume ?? 100;
 
@@ -382,6 +392,15 @@ namespace osu.Game.Beatmaps.Formats
             }
         }
 
-        private string toLegacyCustomSampleBank(string sampleSuffix) => string.IsNullOrEmpty(sampleSuffix) ? "0" : sampleSuffix;
+        private string toLegacyCustomSampleBank(HitSampleInfo hitSampleInfo)
+        {
+            if (hitSampleInfo == null)
+                return "0";
+
+            if (hitSampleInfo is ConvertHitObjectParser.LegacyHitSampleInfo legacy)
+                return legacy.CustomSampleBank.ToString(CultureInfo.InvariantCulture);
+
+            return "0";
+        }
     }
 }

osu.Game/IO/Serialization/Converters/TypedListConverter.cs

@@ -43,13 +43,13 @@ namespace osu.Game.IO.Serialization.Converters
             var list = new List<T>();
 
             var obj = JObject.Load(reader);
-            var lookupTable = serializer.Deserialize<List<string>>(obj["lookup_table"].CreateReader());
+            var lookupTable = serializer.Deserialize<List<string>>(obj["$lookup_table"].CreateReader());
 
-            foreach (var tok in obj["items"])
+            foreach (var tok in obj["$items"])
             {
                 var itemReader = tok.CreateReader();
 
-                var typeName = lookupTable[(int)tok["type"]];
+                var typeName = lookupTable[(int)tok["$type"]];
                 var instance = (T)Activator.CreateInstance(Type.GetType(typeName));
                 serializer.Populate(itemReader, instance);
 
@@ -61,7 +61,7 @@ namespace osu.Game.IO.Serialization.Converters
 
         public override void WriteJson(JsonWriter writer, object value, JsonSerializer serializer)
         {
-            var list = (List<T>)value;
+            var list = (IEnumerable<T>)value;
 
             var lookupTable = new List<string>();
             var objects = new List<JObject>();
@@ -84,16 +84,16 @@ namespace osu.Game.IO.Serialization.Converters
                 }
 
                 var itemObject = JObject.FromObject(item, serializer);
-                itemObject.AddFirst(new JProperty("type", typeId));
+                itemObject.AddFirst(new JProperty("$type", typeId));
                 objects.Add(itemObject);
             }
 
             writer.WriteStartObject();
 
-            writer.WritePropertyName("lookup_table");
+            writer.WritePropertyName("$lookup_table");
             serializer.Serialize(writer, lookupTable);
 
-            writer.WritePropertyName("items");
+            writer.WritePropertyName("$items");
             serializer.Serialize(writer, objects);
 
             writer.WriteEndObject();