Merge pull request #20156 from peppy/sb-earliest-alpha-fix-atttempt-two

Fix some storyboard elements displaying too late due to incorrect start time allowances

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

@@ -117,6 +117,26 @@ namespace osu.Game.Tests.Beatmaps.Formats
             }
         }
 
+        [Test]
+        public void TestEarliestStartTimeWithLoopAlphas()
+        {
+            var decoder = new LegacyStoryboardDecoder();
+
+            using (var resStream = TestResources.OpenResource("loop-containing-earlier-non-zero-fade.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(1000, background.Elements[0].StartTime);
+                Assert.AreEqual(1000, background.Elements[1].StartTime);
+
+                Assert.AreEqual(1000, storyboard.EarliestEventTime);
+            }
+        }
+
         [Test]
         public void TestDecodeVariableWithSuffix()
         {

osu.Game.Tests/Resources/loop-containing-earlier-non-zero-fade.osb

@@ -0,0 +1,14 @@
+osu file format v14
+
+[Events]
+//Storyboard Layer 0 (Background)
+Sprite,Background,TopCentre,"img.jpg",320,240
+ L,1000,1
+  F,0,0,,1       // fade inside a loop with non-zero alpha and an earlier start time should be the true start time..
+ F,0,2000,,0     // ..not a zero alpha fade with a later start time
+
+Sprite,Background,TopCentre,"img.jpg",320,240
+ L,2000,1
+  F,0,0,24,0     // fade inside a loop with zero alpha but later start time than the top-level zero alpha start time.
+  F,0,24,48,1
+ F,0,1000,,1     // ..so this should be the true start time

osu.Game.Tests/Visual/Gameplay/TestSceneLeadIn.cs

@@ -66,18 +66,20 @@ namespace osu.Game.Tests.Visual.Gameplay
         [TestCase(-10000, -10000, true)]
         public void TestStoryboardProducesCorrectStartTimeFadeInAfterOtherEvents(double firstStoryboardEvent, double expectedStartTime, bool addEventToLoop)
         {
+            const double loop_start_time = -20000;
+
             var storyboard = new Storyboard();
 
             var sprite = new StoryboardSprite("unknown", Anchor.TopLeft, Vector2.Zero);
 
             // these should be ignored as we have an alpha visibility blocker proceeding this command.
-            sprite.TimelineGroup.Scale.Add(Easing.None, -20000, -18000, 0, 1);
+            sprite.TimelineGroup.Scale.Add(Easing.None, loop_start_time, -18000, 0, 1);
-            var loopGroup = sprite.AddLoop(-20000, 50);
+            var loopGroup = sprite.AddLoop(loop_start_time, 50);
-            loopGroup.Scale.Add(Easing.None, -20000, -18000, 0, 1);
+            loopGroup.Scale.Add(Easing.None, loop_start_time, -18000, 0, 1);
 
             var target = addEventToLoop ? loopGroup : sprite.TimelineGroup;
-            double targetTime = addEventToLoop ? 20000 : 0;
+            double loopRelativeOffset = addEventToLoop ? -loop_start_time : 0;
-            target.Alpha.Add(Easing.None, targetTime + firstStoryboardEvent, targetTime + firstStoryboardEvent + 500, 0, 1);
+            target.Alpha.Add(Easing.None, loopRelativeOffset + firstStoryboardEvent, loopRelativeOffset + firstStoryboardEvent + 500, 0, 1);
 
             // these should be ignored due to being in the future.
             sprite.TimelineGroup.Alpha.Add(Easing.None, 18000, 20000, 0, 1);

osu.Game/Storyboards/CommandTimelineGroup.cs

@@ -47,30 +47,11 @@ namespace osu.Game.Storyboards
             };
         }
 
-        /// <summary>
-        /// Returns the earliest visible time. Will be null unless this group's first <see cref="Alpha"/> command has a start value of zero.
-        /// </summary>
-        public double? EarliestDisplayedTime
-        {
-            get
-            {
-                var first = Alpha.Commands.FirstOrDefault();
-
-                return first?.StartValue == 0 ? first.StartTime : null;
-            }
-        }
-
         [JsonIgnore]
         public double CommandsStartTime
         {
             get
             {
-                // if the first alpha command starts at zero it should be given priority over anything else.
-                // this is due to it creating a state where the target is not present before that time, causing any other events to not be visible.
-                double? earliestDisplay = EarliestDisplayedTime;
-                if (earliestDisplay != null)
-                    return earliestDisplay.Value;
-
                 double min = double.MaxValue;
 
                 for (int i = 0; i < timelines.Length; i++)

osu.Game/Storyboards/StoryboardSprite.cs

@@ -30,24 +30,35 @@ namespace osu.Game.Storyboards
         {
             get
             {
-                // check for presence affecting commands as an initial pass.
+                // 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.
-                double earliestStartTime = TimelineGroup.EarliestDisplayedTime ?? double.MaxValue;
+                // A StartValue of zero governs, above all else, the first valid display time of a sprite.
+                //
+                // 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)>();
 
-                foreach (var l in loops)
+                var command = TimelineGroup.Alpha.Commands.FirstOrDefault();
+                if (command != null) alphaCommands.Add((command.StartTime, command.StartValue == 0));
+
+                foreach (var loop in loops)
                 {
-                    if (l.EarliestDisplayedTime is double loopEarliestDisplayTime)
+                    command = loop.Alpha.Commands.FirstOrDefault();
-                        earliestStartTime = Math.Min(earliestStartTime, l.LoopStartTime + loopEarliestDisplayTime);
+                    if (command != null) alphaCommands.Add((command.StartTime + loop.LoopStartTime, command.StartValue == 0));
                 }
 
-                if (earliestStartTime < double.MaxValue)
+                if (alphaCommands.Count > 0)
-                    return earliestStartTime;
+                {
+                    var firstAlpha = alphaCommands.OrderBy(t => t.startTime).First();
 
-                // if an alpha-affecting command was not found, use the earliest of any command.
+                    if (firstAlpha.isZeroStartValue)
-                earliestStartTime = TimelineGroup.StartTime;
+                        return firstAlpha.startTime;
+                }
 
+                // If we got to this point, either no alpha commands were present, or the earliest had a non-zero start value.
+                // The sprite's StartTime will be determined by the earliest command, regardless of type.
+                double earliestStartTime = TimelineGroup.StartTime;
                 foreach (var l in loops)
                     earliestStartTime = Math.Min(earliestStartTime, l.StartTime);
-
                 return earliestStartTime;
             }
         }