From 203e294e490c3821c574d6ef5c66f7c2ce6fc5c9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 25 Mar 2025 16:09:26 +0900 Subject: [PATCH 1/5] Fix storyboards with no-op alpha operations causing extended drawable lifetimes Test with https://osu.ppy.sh/beatmapsets/139525#osu/348550. See https://github.com/ppy/osu/issues/32453#issuecomment-2742562780 (and inline comments) for explanation. Closes https://github.com/ppy/osu/issues/32453. --- osu.Game/Storyboards/StoryboardSprite.cs | 37 ++++++++++++++++-------- 1 file changed, 25 insertions(+), 12 deletions(-) diff --git a/osu.Game/Storyboards/StoryboardSprite.cs b/osu.Game/Storyboards/StoryboardSprite.cs index 42426c8c85..968c2be929 100644 --- a/osu.Game/Storyboards/StoryboardSprite.cs +++ b/osu.Game/Storyboards/StoryboardSprite.cs @@ -28,31 +28,44 @@ namespace osu.Game.Storyboards { get { - // To get the initial start time, we need to check whether the first alpha command to exist (across all loops) has a StartValue of zero. - // A StartValue of zero governs, above all else, the first valid display time of a sprite. + // Users that are crafting storyboards using raw osb scripting or external tools may create alpha events far before the actual display time + // of sprites. // - // You can imagine that the first command of each type decides that type's start value, so if the initial alpha is zero, - // anything before that point can be ignored (the sprite is not visible after all). - var alphaCommands = new List<(double startTime, bool isZeroStartValue)>(); + // To make sure lifetime optimisations work as efficiently as they can, let's locally find the first time a sprite becomes visible. + var alphaCommands = new List>(); - var command = Commands.Alpha.FirstOrDefault(); - if (command != null) alphaCommands.Add((command.StartTime, command.StartValue == 0)); + foreach (var command in Commands.Alpha) + { + alphaCommands.Add(command); + if (visibleAtStartOrEnd(command)) + break; + } foreach (var loop in loopingGroups) { - command = loop.Alpha.FirstOrDefault(); - if (command != null) alphaCommands.Add((command.StartTime, command.StartValue == 0)); + foreach (var command in loop.Alpha) + { + alphaCommands.Add(command); + if (visibleAtStartOrEnd(command)) + break; + } } if (alphaCommands.Count > 0) { - var firstAlpha = alphaCommands.MinBy(t => t.startTime); + // Special care is given to cases where there's one or more no-op transforms (ie transforming from alpha 0 to alpha 0). + // - If a 0->0 transform exists, we still need to check it to ensure the absolute first start value is non-visible. + // - After ascertaining this, we then check the first non-noop transform to get the true start lifetime. + var firstAlpha = alphaCommands.MinBy(c => c.StartTime); + var firstRealAlpha = alphaCommands.Where(visibleAtStartOrEnd).MinBy(c => c.StartTime); - if (firstAlpha.isZeroStartValue) - return firstAlpha.startTime; + if (firstAlpha!.StartValue == 0) + return firstRealAlpha!.StartTime; } return EarliestTransformTime; + + bool visibleAtStartOrEnd(StoryboardCommand command) => command.StartValue > 0 || command.EndValue > 0; } } From dd7026f7c71e422e775d73f06e0ec9de145ba7f5 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 26 Mar 2025 19:40:19 +0900 Subject: [PATCH 2/5] Fix test failures due to `StoryboarVideo` having a weird initialisation flow --- osu.Game/Storyboards/StoryboardSprite.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Storyboards/StoryboardSprite.cs b/osu.Game/Storyboards/StoryboardSprite.cs index 968c2be929..49fa5d85c3 100644 --- a/osu.Game/Storyboards/StoryboardSprite.cs +++ b/osu.Game/Storyboards/StoryboardSprite.cs @@ -59,8 +59,8 @@ namespace osu.Game.Storyboards var firstAlpha = alphaCommands.MinBy(c => c.StartTime); var firstRealAlpha = alphaCommands.Where(visibleAtStartOrEnd).MinBy(c => c.StartTime); - if (firstAlpha!.StartValue == 0) - return firstRealAlpha!.StartTime; + if (firstAlpha!.StartValue == 0 && firstRealAlpha != null) + return firstRealAlpha.StartTime; } return EarliestTransformTime; From 4b62ea8d7402501b5a9e4041ab9279f1a44eaa83 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 26 Mar 2025 19:45:54 +0900 Subject: [PATCH 3/5] Add test coverage of previous failcase --- .../Formats/LegacyStoryboardDecoderTest.cs | 18 ++++++++++++++++++ ...-fade-transform-is-ignored-for-lifetime.osb | 8 ++++++++ 2 files changed, 26 insertions(+) create mode 100644 osu.Game.Tests/Resources/noop-fade-transform-is-ignored-for-lifetime.osb diff --git a/osu.Game.Tests/Beatmaps/Formats/LegacyStoryboardDecoderTest.cs b/osu.Game.Tests/Beatmaps/Formats/LegacyStoryboardDecoderTest.cs index 647c0aed75..821173c521 100644 --- a/osu.Game.Tests/Beatmaps/Formats/LegacyStoryboardDecoderTest.cs +++ b/osu.Game.Tests/Beatmaps/Formats/LegacyStoryboardDecoderTest.cs @@ -135,6 +135,24 @@ namespace osu.Game.Tests.Beatmaps.Formats } } + [Test] + public void TestNoopFadeTransformIsIgnoredForLifetime() + { + var decoder = new LegacyStoryboardDecoder(); + + using (var resStream = TestResources.OpenResource("noop-fade-transform-is-ignored-for-lifetime.osb")) + using (var stream = new LineBufferedReader(resStream)) + { + var storyboard = decoder.Decode(stream); + + StoryboardLayer background = storyboard.Layers.Single(l => l.Depth == 3); + Assert.AreEqual(2, background.Elements.Count); + + Assert.AreEqual(1500, background.Elements[0].StartTime); + Assert.AreEqual(1500, background.Elements[1].StartTime); + } + } + [Test] public void TestOutOfOrderStartTimes() { diff --git a/osu.Game.Tests/Resources/noop-fade-transform-is-ignored-for-lifetime.osb b/osu.Game.Tests/Resources/noop-fade-transform-is-ignored-for-lifetime.osb new file mode 100644 index 0000000000..aca9bf926a --- /dev/null +++ b/osu.Game.Tests/Resources/noop-fade-transform-is-ignored-for-lifetime.osb @@ -0,0 +1,8 @@ +[Events] +//Storyboard Layer 0 (Background) +Sprite,Background,TopCentre,"img.jpg",320,240 + F,0,1000,1000,0,0 // should be ignored + F,0,1500,1600,0,1 +Sprite,Background,TopCentre,"img.jpg",320,240 + F,0,1000,1000,0,0 // should be ignored + F,0,1500,1600,1,1 From a49f9be243db5c2801afc9372bf79bcbce8b485a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 3 Apr 2025 18:02:57 +0900 Subject: [PATCH 4/5] Add failing test coverage of regressing video scenario --- .../Formats/LegacyStoryboardDecoderTest.cs | 23 +++++++++++++++++++ .../video-custom-alpha-transform.osb | 5 ++++ 2 files changed, 28 insertions(+) create mode 100644 osu.Game.Tests/Resources/video-custom-alpha-transform.osb diff --git a/osu.Game.Tests/Beatmaps/Formats/LegacyStoryboardDecoderTest.cs b/osu.Game.Tests/Beatmaps/Formats/LegacyStoryboardDecoderTest.cs index 821173c521..b10cce6a52 100644 --- a/osu.Game.Tests/Beatmaps/Formats/LegacyStoryboardDecoderTest.cs +++ b/osu.Game.Tests/Beatmaps/Formats/LegacyStoryboardDecoderTest.cs @@ -306,6 +306,29 @@ namespace osu.Game.Tests.Beatmaps.Formats } } + [Test] + public void TestVideoWithCustomFadeIn() + { + var decoder = new LegacyStoryboardDecoder(); + + using var resStream = TestResources.OpenResource("video-custom-alpha-transform.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(((StoryboardVideo)storyboard.GetLayer(@"Video").Elements.Single()).Commands.Alpha.Single().StartTime, Is.EqualTo(1500)); + Assert.That(((StoryboardVideo)storyboard.GetLayer(@"Video").Elements.Single()).Commands.Alpha.Single().EndTime, Is.EqualTo(1600)); + + Assert.That(storyboard.EarliestEventTime, Is.Null); + Assert.That(storyboard.LatestEventTime, Is.Null); + }); + } + [Test] public void TestVideoAndBackgroundEventsDoNotAffectStoryboardBounds() { diff --git a/osu.Game.Tests/Resources/video-custom-alpha-transform.osb b/osu.Game.Tests/Resources/video-custom-alpha-transform.osb new file mode 100644 index 0000000000..39fcf87c06 --- /dev/null +++ b/osu.Game.Tests/Resources/video-custom-alpha-transform.osb @@ -0,0 +1,5 @@ +osu file format v14 + +[Events] +Video,-5678,"Video.avi",0,0 + F,0,1500,1600,0,1 From 102085668f84bd80f1717f101adc22fc7075e7fa Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Thu, 3 Apr 2025 18:03:23 +0900 Subject: [PATCH 5/5] Fix video offset start time regression with `StartTime` calculation changes --- osu.Game/Storyboards/StoryboardSprite.cs | 2 +- osu.Game/Storyboards/StoryboardVideo.cs | 4 +++- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/osu.Game/Storyboards/StoryboardSprite.cs b/osu.Game/Storyboards/StoryboardSprite.cs index 49fa5d85c3..e10edfefe1 100644 --- a/osu.Game/Storyboards/StoryboardSprite.cs +++ b/osu.Game/Storyboards/StoryboardSprite.cs @@ -24,7 +24,7 @@ namespace osu.Game.Storyboards public readonly StoryboardCommandGroup Commands = new StoryboardCommandGroup(); - public double StartTime + public virtual double StartTime { get { diff --git a/osu.Game/Storyboards/StoryboardVideo.cs b/osu.Game/Storyboards/StoryboardVideo.cs index 14189a1a6c..fb4ac56e98 100644 --- a/osu.Game/Storyboards/StoryboardVideo.cs +++ b/osu.Game/Storyboards/StoryboardVideo.cs @@ -14,9 +14,11 @@ namespace osu.Game.Storyboards { // This is just required to get a valid StartTime based on the incoming offset. // Actual fades are handled inside DrawableStoryboardVideo for now. - Commands.AddAlpha(Easing.None, offset, offset, 0, 0); + StartTime = offset; } + public override double StartTime { get; } + public override Drawable CreateDrawable() => new DrawableStoryboardVideo(this); } }