Preserve storyboard events when saving a beatmap in the editor

[peppy]

Until we have full encoding support for storyboards, this stop-gap measure ensures that storyboards don't just disappear from existence.

Without this, saving a beatmap in the editor removes all local storyboard content (ie. anything in the .osu file). The most noticeable is any video background.

[bdach]

I don't think this is going to fully preserve the storyboard because of

osu/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs

Line 170 in 0c45a48

protected override bool ShouldSkipLine(string line) => base.ShouldSkipLine(line) || line.StartsWith(' ') || line.StartsWith('_');

The above means that any commands are not even attempted to be read, and will be silently dropped.

One example to test on is https://osu.ppy.sh/beatmapsets/1049855#osu/2264046 (of #27967 fame). Saving this map in the editor causes the background video to fill the entire playfield area rather than use its native size.

This is still an improvement mind, but I figure I should bring this up as part of review.

[bdach]

ba9f4e4 is still not enough, because with it this happens:

[runtime] 2024-04-30 12:29:15 [verbose]: Failed to process line " S,0,0,,1" into "Frums - Options (Nerova Riuz GX) [Option 3: Hyper]": Unknown event type:  S

which comes from

osu/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs

Lines 420 to 421 in 5e2e2a8

	if (!Enum.TryParse(split[0], out LegacyEventType type))
	throw new InvalidDataException($@"Unknown event type: {split[0]}");

Honestly for now I think the best choice would be to not play the "unhandled lines" game (because I worry about other ways that can go wrong, by e.g. reordering lines on accident) and just store everything as it came in. As in

diff --git a/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapEncoderTest.cs b/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapEncoderTest.cs
index b931896898..9e27bfbd3e 100644
--- a/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapEncoderTest.cs
+++ b/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapEncoderTest.cs
@@ -44,8 +44,8 @@ public void TestUnsupportedStoryboardEvents()
             const string name = "Resources/storyboard_only_video.osu";
 
             var decoded = decodeFromLegacy(beatmaps_resource_store.GetStream(name), name);
-            Assert.That(decoded.beatmap.UnhandledEventLines.Count, Is.EqualTo(1));
-            Assert.That(decoded.beatmap.UnhandledEventLines.Single(), Is.EqualTo("Video,0,\"video.avi\""));
+            Assert.That(decoded.beatmap.EventLines.Count, Is.EqualTo(1));
+            Assert.That(decoded.beatmap.EventLines.Single(), Is.EqualTo("Video,0,\"video.avi\""));
 
             var memoryStream = encodeToLegacy(decoded);
 
diff --git a/osu.Game/Beatmaps/Beatmap.cs b/osu.Game/Beatmaps/Beatmap.cs
index ae77e4adcf..ac0b607f30 100644
--- a/osu.Game/Beatmaps/Beatmap.cs
+++ b/osu.Game/Beatmaps/Beatmap.cs
@@ -63,7 +63,7 @@ public Beatmap()
 
         public List<BreakPeriod> Breaks { get; set; } = new List<BreakPeriod>();
 
-        public List<string> UnhandledEventLines { get; set; } = new List<string>();
+        public List<string> EventLines { get; set; } = new List<string>();
 
         [JsonIgnore]
         public double TotalBreakTime => Breaks.Sum(b => b.Duration);
diff --git a/osu.Game/Beatmaps/BeatmapConverter.cs b/osu.Game/Beatmaps/BeatmapConverter.cs
index b68c80d4b3..74fa614988 100644
--- a/osu.Game/Beatmaps/BeatmapConverter.cs
+++ b/osu.Game/Beatmaps/BeatmapConverter.cs
@@ -66,7 +66,7 @@ protected virtual Beatmap<T> ConvertBeatmap(IBeatmap original, CancellationToken
             beatmap.ControlPointInfo = original.ControlPointInfo;
             beatmap.HitObjects = convertHitObjects(original.HitObjects, original, cancellationToken).OrderBy(s => s.StartTime).ToList();
             beatmap.Breaks = original.Breaks;
-            beatmap.UnhandledEventLines = original.UnhandledEventLines;
+            beatmap.EventLines = original.EventLines;
 
             return beatmap;
         }
diff --git a/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs b/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs
index 84f3c0d82a..e05ba3efc0 100644
--- a/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs
+++ b/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs
@@ -413,15 +413,15 @@ private void handleDifficulty(string line)
 
         private void handleEvent(string line)
         {
+            beatmap.EventLines.Add(line);
             string[] split = line.Split(',');
 
+            if (line.StartsWith(' ') || line.StartsWith('_'))
+                return;
+
             if (!Enum.TryParse(split[0], out LegacyEventType type))
                 throw new InvalidDataException($@"Unknown event type: {split[0]}");
 
-            // Until we have full storyboard encoder coverage, let's track any lines which aren't handled
-            // and store them to a temporary location such that they aren't lost on editor save / export.
-            bool lineSupportedByEncoder = false;
-
             switch (type)
             {
                 case LegacyEventType.Sprite:
@@ -429,10 +429,7 @@ private void handleEvent(string line)
                     // In some older beatmaps, it is not present and replaced by a storyboard-level background instead.
                     // Allow the first sprite (by file order) to act as the background in such cases.
                     if (string.IsNullOrEmpty(beatmap.BeatmapInfo.Metadata.BackgroundFile))
-                    {
                         beatmap.BeatmapInfo.Metadata.BackgroundFile = CleanFilename(split[3]);
-                        lineSupportedByEncoder = true;
-                    }
 
                     break;
 
@@ -443,16 +440,12 @@ private void handleEvent(string line)
                     // instead of 0 for BACKGROUND). To handle this gracefully, check the file extension against known supported
                     // video extensions and handle similar to a background if it doesn't match.
                     if (!OsuGameBase.VIDEO_EXTENSIONS.Contains(Path.GetExtension(filename).ToLowerInvariant()))
-                    {
                         beatmap.BeatmapInfo.Metadata.BackgroundFile = filename;
-                        lineSupportedByEncoder = true;
-                    }
 
                     break;
 
                 case LegacyEventType.Background:
                     beatmap.BeatmapInfo.Metadata.BackgroundFile = CleanFilename(split[2]);
-                    lineSupportedByEncoder = true;
                     break;
 
                 case LegacyEventType.Break:
@@ -460,12 +453,8 @@ private void handleEvent(string line)
                     double end = Math.Max(start, getOffsetTime(Parsing.ParseDouble(split[2])));
 
                     beatmap.Breaks.Add(new BreakPeriod(start, end));
-                    lineSupportedByEncoder = true;
                     break;
             }
-
-            if (!lineSupportedByEncoder)
-                beatmap.UnhandledEventLines.Add(line);
         }
 
         private void handleTimingPoint(string line)
diff --git a/osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs b/osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs
index 186b565c39..99ee323fba 100644
--- a/osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs
+++ b/osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs
@@ -151,13 +151,7 @@ private void handleEvents(TextWriter writer)
         {
             writer.WriteLine("[Events]");
 
-            if (!string.IsNullOrEmpty(beatmap.BeatmapInfo.Metadata.BackgroundFile))
-                writer.WriteLine(FormattableString.Invariant($"{(int)LegacyEventType.Background},0,\"{beatmap.BeatmapInfo.Metadata.BackgroundFile}\",0,0"));
-
-            foreach (var b in beatmap.Breaks)
-                writer.WriteLine(FormattableString.Invariant($"{(int)LegacyEventType.Break},{b.StartTime},{b.EndTime}"));
-
-            foreach (string l in beatmap.UnhandledEventLines)
+            foreach (string l in beatmap.EventLines)
                 writer.WriteLine(l);
         }
 
diff --git a/osu.Game/Beatmaps/IBeatmap.cs b/osu.Game/Beatmaps/IBeatmap.cs
index 5cc38e5b84..e59a97b29d 100644
--- a/osu.Game/Beatmaps/IBeatmap.cs
+++ b/osu.Game/Beatmaps/IBeatmap.cs
@@ -43,10 +43,10 @@ public interface IBeatmap
         List<BreakPeriod> Breaks { get; }
 
         /// <summary>
-        /// All lines from the [Events] section which aren't handled in the encoding process yet.
+        /// All lines from the [Events] section.
         /// These lines shoule be written out to the beatmap file on save or export.
         /// </summary>
-        List<string> UnhandledEventLines { get; }
+        List<string> EventLines { get; }
 
         /// <summary>
         /// Total amount of break time in the beatmap.
diff --git a/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs b/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs
index d37cfc28b9..b04ef8d259 100644
--- a/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs
+++ b/osu.Game/Rulesets/Difficulty/DifficultyCalculator.cs
@@ -330,7 +330,7 @@ public BeatmapDifficulty Difficulty
             }
 
             public List<BreakPeriod> Breaks => baseBeatmap.Breaks;
-            public List<string> UnhandledEventLines => baseBeatmap.UnhandledEventLines;
+            public List<string> EventLines => baseBeatmap.EventLines;
 
             public double TotalBreakTime => baseBeatmap.TotalBreakTime;
             public IEnumerable<BeatmapStatistic> GetStatistics() => baseBeatmap.GetStatistics();
diff --git a/osu.Game/Screens/Edit/EditorBeatmap.cs b/osu.Game/Screens/Edit/EditorBeatmap.cs
index 7a3ea474fb..dda65a1c50 100644
--- a/osu.Game/Screens/Edit/EditorBeatmap.cs
+++ b/osu.Game/Screens/Edit/EditorBeatmap.cs
@@ -174,7 +174,7 @@ public ControlPointInfo ControlPointInfo
 
         public List<BreakPeriod> Breaks => PlayableBeatmap.Breaks;
 
-        public List<string> UnhandledEventLines => PlayableBeatmap.UnhandledEventLines;
+        public List<string> EventLines => PlayableBeatmap.EventLines;
 
         public double TotalBreakTime => PlayableBeatmap.TotalBreakTime;
 
diff --git a/osu.Game/Tests/Beatmaps/TestBeatmap.cs b/osu.Game/Tests/Beatmaps/TestBeatmap.cs
index de7bcfcfaa..d809d938a2 100644
--- a/osu.Game/Tests/Beatmaps/TestBeatmap.cs
+++ b/osu.Game/Tests/Beatmaps/TestBeatmap.cs
@@ -27,7 +27,7 @@ public TestBeatmap(RulesetInfo ruleset, bool withHitObjects = true)
             BeatmapInfo = baseBeatmap.BeatmapInfo;
             ControlPointInfo = baseBeatmap.ControlPointInfo;
             Breaks = baseBeatmap.Breaks;
-            UnhandledEventLines = baseBeatmap.UnhandledEventLines;
+            EventLines = baseBeatmap.EventLines;
 
             if (withHitObjects)
                 HitObjects = baseBeatmap.HitObjects;

[peppy]

The reason I didn't do that is it will break changing backgrounds / breaks in the editor:

I guess you could argue that break editing isn't supported yet, but the background line would still not be updated by any editor changes, so some custom handling would be required here, no?

[bdach]

Oh... fair point it would yeah.

I dunno where's our pain threshold at then. With things how they are here, you'd probably have to either drop commands attached to those background/video events that get special handling in the encoder right now, or somehow associate them with the background/video and reinsert them in the correct place. Not sure. It seems that both would be similar levels of hassle due to state-keeping, so might as well try for correct...?

[peppy]

Seems like we can get away with part of your patch.

After export now:

As for lines getting disassociated (ie. transforms) I don't think that is an issue with our current level of storyboard support, because transforms attached to the video will be read and added back in sequential order. Others shouldn't matter too much.

Probably just keep it simple for now while it just-works?

[bdach]

Probably just keep it simple for now while it just-works?

Yeah let's just go with this for now. If it's insufficient we'll inevitably learn about it anyways.