ppy / osu

rhythm is just a *click* away!
https://osu.ppy.sh
MIT License
15.37k stars 2.29k forks source link

Storyboard section with loops rendered incorrectly #29675

Open NoffyNoffeh opened 2 months ago

NoffyNoffeh commented 2 months ago

Type

Game Behavior

Bug description

When playing the daily challenge for the Hide and Seek set https://osu.ppy.sh/beatmapsets/972932#osu/2036905 , I noticed my storyboard is broken in lazer at 01:02:005 - through 01:07:895 - and 01:55:011 -

Some elements expected to scale repeatedly using a loop command stop scaling after the first instance of the loop command is completed, and creates a different undesired visual when compared to stable.

Screenshots or videos

A video showing how it currently plays in stable vs. lazer: https://www.youtube.com/watch?v=qjzQ3kqhi6Y

Here is the osb script for the two related sprites, if it's useful to reference:

bottom sprite image

top sprite with looping scale command image

Version

2024.817.0

Logs

compressed-logs.zip

bdach commented 1 month ago

Looking at what the storyboard classes do, the minimal example is

        [Test]
        public void TestTest() => AddStep("do stuff", () =>
        {
            Box box;

            Child = box = new MyBox
            {
                Size = new Vector2(100),
                Anchor = Anchor.Centre,
                Origin = Anchor.Centre,
            };

            using (box.BeginDelayedSequence(500))
                box.ScaleTo(Vector2.Zero).Then().ScaleTo(new Vector2(0.8f), 250, Easing.OutElastic).Loop(500, 10);

            using (box.BeginDelayedSequence(1000))
                box.ScaleTo(Vector2.Zero).Then().ScaleTo(Vector2.Zero).Loop(500, 10);
        });

        private partial class MyBox : Box
        {
            public override bool RemoveCompletedTransforms => false;
        }

(MyBox is there just to be able to see the transforms in post, although it being present does match the storyboard case.)

I'm not super sure where to take this next, because you could argue that it's a framework bug in transform handling, or it's a bug in the storyboard implementation because this code is not valid.

@smoogipoo mind taking a look here and chipping in as to whether you'd expect the code as written above to visually behave as is expected? (see video in OP)

peppy commented 1 month ago

In the repro you provided, is the issue that the two loops are overlapping each other?

bdach commented 1 month ago

Something like that. Video for reference:

https://github.com/user-attachments/assets/be68bc67-9d2b-4f75-a202-b752465272c4

I haven't looked in detail what is happening precisely but it appears the two loops interfere with each other in a way that affects the correct readout of initial values of the transform. Hence the question as to whether this is even considered valid usage from the framework's standpoint.

Wieku commented 1 month ago

@bdach Not a solution, but some insights.

Hence the question as to whether this is even considered valid usage from the framework's standpoint

Pretty sure not. Transform system keeps track of the latest transform and discards older ones: https://github.com/ppy/osu-framework/blob/e4cbe949e87e6d72fbbcfca07a545abe8a1c1a37/osu.Framework/Graphics/Transforms/TargetGroupingTransformTracker.cs#L121-L151

Transforms are ordered by start time and then by transform id, so if both loop elements have same absolute start times then the one with bigger id (added later) takes precedence.

If we add instant commands first then it executes as expected (for some reason sharex freaks out when recording tests):

https://github.com/user-attachments/assets/e4938057-e39f-4164-a03f-cf722fa97ec6

using (box.BeginDelayedSequence(500))
    box.ScaleTo(Vector2.Zero).Then().ScaleTo(new Vector2(0.8f), 552, Easing.OutBack).Loop(184, 10);

using (box.BeginDelayedSequence(1236))
    box.ScaleTo(Vector2.Zero).Then().ScaleTo(Vector2.Zero).Loop(736, 10);

So if changing the framework is out of the question, storyboard parser would have to recognize instant commands at the end of the loop and insert them before the rest.