From 2f9b50172e0b0f261a94585d2c525405a77eaa48 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 27 Oct 2023 22:09:33 +0200 Subject: [PATCH 1/3] Add failing test coverage for video events affecting storyboard time bounds --- .../Formats/LegacyStoryboardDecoderTest.cs | 21 +++++++++++++++++++ .../video-background-events-ignored.osb | 5 +++++ 2 files changed, 26 insertions(+) create mode 100644 osu.Game.Tests/Resources/video-background-events-ignored.osb diff --git a/osu.Game.Tests/Beatmaps/Formats/LegacyStoryboardDecoderTest.cs b/osu.Game.Tests/Beatmaps/Formats/LegacyStoryboardDecoderTest.cs index 34ff8bfd84..647c0aed75 100644 --- a/osu.Game.Tests/Beatmaps/Formats/LegacyStoryboardDecoderTest.cs +++ b/osu.Game.Tests/Beatmaps/Formats/LegacyStoryboardDecoderTest.cs @@ -287,5 +287,26 @@ namespace osu.Game.Tests.Beatmaps.Formats Assert.That(manyTimes.EndTime, Is.EqualTo(9000 + loop_duration)); } } + + [Test] + public void TestVideoAndBackgroundEventsDoNotAffectStoryboardBounds() + { + var decoder = new LegacyStoryboardDecoder(); + + using var resStream = TestResources.OpenResource("video-background-events-ignored.osb"); + using var stream = new LineBufferedReader(resStream); + + var storyboard = decoder.Decode(stream); + + Assert.Multiple(() => + { + Assert.That(storyboard.GetLayer(@"Video").Elements, Has.Count.EqualTo(1)); + Assert.That(storyboard.GetLayer(@"Video").Elements.Single(), Is.InstanceOf()); + Assert.That(storyboard.GetLayer(@"Video").Elements.Single().StartTime, Is.EqualTo(-5678)); + + Assert.That(storyboard.EarliestEventTime, Is.Null); + Assert.That(storyboard.LatestEventTime, Is.Null); + }); + } } } diff --git a/osu.Game.Tests/Resources/video-background-events-ignored.osb b/osu.Game.Tests/Resources/video-background-events-ignored.osb new file mode 100644 index 0000000000..7525b1fee9 --- /dev/null +++ b/osu.Game.Tests/Resources/video-background-events-ignored.osb @@ -0,0 +1,5 @@ +osu file format v14 + +[Events] +0,-1234,"BG.jpg",0,0 +Video,-5678,"Video.avi",0,0 \ No newline at end of file From 9ce2c1f49c37e63d3d6c18b8fb854ee3b4766adf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Fri, 27 Oct 2023 22:10:03 +0200 Subject: [PATCH 2/3] Exclude video events from being accounted for when calculating storyboard time bounds Closes https://github.com/ppy/osu/issues/25263. In some circumstances, stable allows skipping twice if a particularly long storyboarded intro is being displayed: https://github.com/peppy/osu-stable-reference/blob/master/osu!/GameModes/Play/Player.cs#L1728-L1736 `AllowDoubleSkip` is calculated thus: https://github.com/peppy/osu-stable-reference/blob/3ea48705eb67172c430371dcfc8a16a002ed0d3d/osu!/GameModes/Play/Player.cs#L1761-L1770 and `leadInTime` is calculated thus: https://github.com/peppy/osu-stable-reference/blob/3ea48705eb67172c430371dcfc8a16a002ed0d3d/osu!/GameModes/Play/Player.cs#L1342-L1351 The key to watch out for here is `{first,last}EventTime`. `EventManager` will calculate it on-the-fly as it adds storyboard elements: https://github.com/peppy/osu-stable-reference/blob/3ea48705eb67172c430371dcfc8a16a002ed0d3d/osu!/GameplayElements/Events/EventManager.cs#L253-L256 However, this pathway is only used for sprite, animation, sample, and break events. Video and background events use the following pathway: https://github.com/peppy/osu-stable-reference/blob/master/osu!/GameplayElements/Events/EventManager.cs#L368 Note that this particular overload does not mutate either bound. Which means that for the purposes of determining where a storyboard starts and ends temporally, a video event's start time is essentially ignored. To reflect that, add a clause that excludes video events from calculations of `{Earliest,Latest}EventTime`. --- osu.Game/Storyboards/Storyboard.cs | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) diff --git a/osu.Game/Storyboards/Storyboard.cs b/osu.Game/Storyboards/Storyboard.cs index 21342831b0..a3137fe1b1 100644 --- a/osu.Game/Storyboards/Storyboard.cs +++ b/osu.Game/Storyboards/Storyboard.cs @@ -30,8 +30,11 @@ namespace osu.Game.Storyboards /// /// /// This iterates all elements and as such should be used sparingly or stored locally. + /// Video and background events are not included to match stable. /// - public double? EarliestEventTime => Layers.SelectMany(l => l.Elements).MinBy(e => e.StartTime)?.StartTime; + public double? EarliestEventTime => Layers.SelectMany(l => l.Elements) + .Where(e => e is not StoryboardVideo) + .MinBy(e => e.StartTime)?.StartTime; /// /// Across all layers, find the latest point in time that a storyboard element ends at. @@ -39,9 +42,12 @@ namespace osu.Game.Storyboards /// /// /// This iterates all elements and as such should be used sparingly or stored locally. - /// Videos and samples return StartTime as their EndTIme. + /// Samples return StartTime as their EndTIme. + /// Video and background events are not included to match stable. /// - public double? LatestEventTime => Layers.SelectMany(l => l.Elements).MaxBy(e => e.GetEndTime())?.GetEndTime(); + public double? LatestEventTime => Layers.SelectMany(l => l.Elements) + .Where(e => e is not StoryboardVideo) + .MaxBy(e => e.GetEndTime())?.GetEndTime(); /// /// Depth of the currently front-most storyboard layer, excluding the overlay layer. From bac306879a4c2965b780c8588f97e95c24e354d0 Mon Sep 17 00:00:00 2001 From: Salman Ahmed Date: Sat, 28 Oct 2023 03:18:13 +0300 Subject: [PATCH 3/3] Minor reword on documentation --- osu.Game/Storyboards/Storyboard.cs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/osu.Game/Storyboards/Storyboard.cs b/osu.Game/Storyboards/Storyboard.cs index a3137fe1b1..8c43b99702 100644 --- a/osu.Game/Storyboards/Storyboard.cs +++ b/osu.Game/Storyboards/Storyboard.cs @@ -30,6 +30,7 @@ namespace osu.Game.Storyboards /// /// /// This iterates all elements and as such should be used sparingly or stored locally. + /// Sample events use their start time as "end time" during this calculation. /// Video and background events are not included to match stable. /// public double? EarliestEventTime => Layers.SelectMany(l => l.Elements) @@ -42,7 +43,7 @@ namespace osu.Game.Storyboards /// /// /// This iterates all elements and as such should be used sparingly or stored locally. - /// Samples return StartTime as their EndTIme. + /// Sample events use their start time as "end time" during this calculation. /// Video and background events are not included to match stable. /// public double? LatestEventTime => Layers.SelectMany(l => l.Elements)