Re-applying existing transforms from future with different values results in an undefined state

[frenzibyte]

Consider the following scenario:

        [Test]
        public void TestPastTransformDoesntAffectOthers()
        {
            boxTest(box =>
            {
                box.Alpha = 0;
                box.Scale = new Vector2(2);
                box.RemoveCompletedTransforms = false;
            });

            // move forward to future point in time before adding transforms.
            checkAtTime(interval * 4, _ => true);

            AddStep("add transforms", () =>
            {
                using (box.BeginAbsoluteSequence(0))
                {
                    box.FadeIn();
                    box.ScaleTo(1, interval * 4);
                }
            });

            AddStep("change box scale", () => box.Scale = new Vector2(3));

            AddStep("re-apply transforms", () =>
            {
                using (box.BeginAbsoluteSequence(0))
                {
                    // fade-in is added here to test whether applying this transform would reset the state of the Scale property to the values of the previous transform.
                    box.FadeIn();
                    box.ScaleTo(1, interval * 4);
                }
            });

            checkAtTime(0, box => box.Scale == new Vector2(3));
            checkAtTime(interval * 4, box => box.Scale == new Vector2(1));
        }

This fails and ends up with unexpected values when rewinding to time = 0 due to two different reasons:

1. When transforms are re-applied to a drawable with existing transforms, the first transform added will trigger a full update pass on the drawable, causing its current state to be lost and overwritten with the transforms currently existing.
2. When a transform is applied on a certain property with a start time matching an existing transform, the previous one remains and rewinding would favour the previous transform. This is expected on a sequence-based use case (drawable.FadeTo(0.5f).FadeTo(1, 200)), but it probably shouldn't happen on isolated cases (drawable.FadeTo(0.5f) ... drawable.FadeTo(0.25f)).

Opening this issue thread as it resulted in going back and fourth over resolving ppy/osu#20807 (see ppy/osu#20825). And given that there are existing logic for removing transforms that exist after an early new one, it would be expected that this would be implicitly handled without the consumer performing certain operations beforehand.

The alternative and simplest solution would be to clear all existing transforms before applying new ones, but if that's the path we want to go with rather than going through the issues here, then one would hope to be warned about with runtime exceptions (potentially a guard in BeginAbsoluteSequence(t) that throws when there are existing transforms after the specified time).

[smoogipoo]

I would remove the FadeIn() from the example, as it's only a source of confusion. There's no "testing" going on as the comment says, and keeping track of 7 states (of scale alone) is already complicated enough.

[smoogipoo]

This isn't the first time this has come up. Part of the reason we use transforms in DHOs to set initial states is to avoid this exact issue, and we've discussed having e.g. Alpha_set() clear the transform in the past but ultimately decided not to do that. I don't remember the reason why.

I believe the problem with the proposal is, if you have the following piece of code:

drawable.FadeOut();
drawable.FadeIn(3000);

That you would expect the drawable to start at 0 alpha and fade in over 3s, i.e. you'd expect the initial state of that second transform to have Alpha = 0. So "clear all existing transforms" sounds like it would break this, and it is in fact correct to apply existing transforms.

Imo the proper solution is to have these properties' setters clear the respective transforms regardless of time. The best way I could describe it is you have a global state (Colour), and a bunch of intermediate states (this.FadeColour()) that derive from that global state such that you should be able to take any intermediate state and reach the global state it was derived from. But as soon as the global state changes, every one of those intermediate states becomes invalid and can never be used to reach the new global state.

One possible adjustment to this is that you could represent the new global state via a new intermediate state if you clear everything from the current point in time onwards. For example if you have transforms go:
(t:0,a:1) -> (t:1000,a:0.5) -> (t:2000,a:0)
and you set the new global state a:0 at t:1500, you could do this sort of manipulation:
(t:0,a:1) -> (t:1000,a:0.5) -> (t:1500,a:0.25) -> (t:1500,a:0)
The problem is... This only works nicely for linear transforms. To make this sort of thing work regardless of easing, I can only imagine transforms having an additional parameter representing their cutoff time distinctly from their end time.

[frenzibyte]

I would remove the FadeIn() from the example, as it's only a source of confusion. There's no "testing" going on as the comment says, and keeping track of 7 states (of scale alone) is already complicated enough.

The comment was outdated, but the idea was that performing this irrelevant transform with master will end up resetting the value of Scale, which is unexpected since that transform has nothing to do with it, but it's happening due to AddTransform performing a full update pass on all existing transforms.

Imo the proper solution is to have these properties' setters clear the respective transforms regardless of time. The best way I could describe it is you have a global state (Colour), and a bunch of intermediate states (this.FadeColour()) that derive from that global state such that you should be able to take any intermediate state and reach the global state it was derived from. But as soon as the global state changes, every one of those intermediate states becomes invalid and can never be used to reach the new global state.

Can agree with that, but putting it into practice is going to require quite some thought since we currently perform transforms on what would be the "global state" itself. Maybe for starters it should just check whether the target property has changed value from the current state of the transform and then abort it?

[smoogipoo]

performing this irrelevant transform with master will end up resetting the value of Scale

Fair enough.

Maybe for starters it should just check whether the target property has changed value from the current state of the transform and then abort it?

Yeah, it's gonna require quite a bit of thinking for how to get my proposal working nicely (in a performant way).