Single-frame layout issue with nested child dependencies

[smoogipoo]

Description

A single-frame layout discrepancy may be observed when nesting containers that are invalidated by their children.

Case 1 - autosize in autosize: https://drive.google.com/open?id=1T_MGjtFTowFU6OAD1BhpN-_fRG29U8Bi In this case the parenting auto-size is invalidated when the inner autosize is updated (at the end of the frame).

Case 2 autosize in fill flow: https://drive.google.com/open?id=1ePqGK-hArak5mIbDv0Khi-OsQ1LrsVmw In this case, the parenting fill flow layout is invalidated when the inner autosize is updated (at the end of the frame).

Both of these test cases can be found below.

This isn't a regression from the recent layout changes, but has always been the case for performance reasons.

Path forward

The reason this occurs is because the child's invalidation only propagates upwards one level - to the child's immediate parent.

There are two changes required for this:

In CompositeDrawable.cs:

- // Child invalidations should not propagate to other children.
- if (source == InvalidationSource.Child)
-     return anyInvalidated;

In Drawable.cs:

- if (source == InvalidationSource.Self)
+ if (source == InvalidationSource.Self || source == InvalidationSource.Child)
    Parent?.Invalidate(invalidation, InvalidationSource.Child);

However we can't do this without first considering the performance implications, and I'm not really sure why the CompositeDrawable change is required in the first place.

We likely can't fix this without first removing OnInvalidate() completely, and turning it into something delegate-based. A likely path forward while maintaining high performance is to store a reverse enum of child invalidation type dependencies - that is, which invalidation types any of a drawable's supertree cares about.

Test scene

public class TestSceneLayoutIssue : FrameworkTestScene
{
    [Test]
    public void TestCase1()
    {
        TestContainer t = null;

        AddStep("create test", () =>
        {
            Child = new Container
            {
                Anchor = Anchor.Centre,
                Origin = Anchor.Centre,
                AutoSizeAxes = Axes.Both,
                Children = new Drawable[]
                {
                    new Box
                    {
                        RelativeSizeAxes = Axes.Both,
                        Colour = Color4.SlateGray
                    },
                    t = new TestContainer()
                }
            };
        });

        AddStep("set some text", () => t.Text = "whoops!");
    }

    [Test]
    public void TestCase2()
    {
        TestContainer t = null;

        AddStep("create test", () =>
        {
            Child = new Container
            {
                Anchor = Anchor.Centre,
                Origin = Anchor.Centre,
                Size = new Vector2(200), // Assume potentially given by other children.
                Children = new Drawable[]
                {
                    new Box
                    {
                        RelativeSizeAxes = Axes.Both,
                        Colour = Color4.SlateGray
                    },
                    new FillFlowContainer
                    {
                        RelativeSizeAxes = Axes.Both,
                        Child = t = new TestContainer
                        {
                            Anchor = Anchor.TopCentre,
                            Origin = Anchor.TopCentre,
                        }
                    }
                }
            };
        });

        AddStep("set some text", () => t.Text = "whoops!");
    }

    private class TestContainer : CompositeDrawable
    {
        public string Text
        {
            get => text.Text;
            set => text.Text = value;
        }

        private readonly SpriteText text;

        public TestContainer()
        {
            AutoSizeAxes = Axes.Both;

            InternalChild = text = new SpriteText();
        }
    }
}

[ekrctb]

Adding Invalidation.DrawSize to this Invalidate in SpriteText

https://github.com/ppy/osu-framework/blob/c58cd8294a5868dbab07529b51222087a18019f3/osu.Framework/Graphics/Sprites/SpriteText.cs#L558

seems to fix the presented test cases (probably should be invalidated only if autosize). And it makes sense because the size is going to change when the text is changed. Another test case that is fixed by this change is:

[Test]
public void TestCaseSpriteTextInvalidationPropagation()
{
    SpriteText text = null;
    AddStep("create test", () =>
    {
        Child = new Container
        {
            AutoSizeAxes = Axes.Both,
            Child = text = new SpriteText()
        };
    });
    AddStep("modify text", () =>
    {
        text.Text = "xxx";
        // Parent size is stale.
        Console.WriteLine($"Parent size = {text.Parent.Size}");
        // Get the text size.
        Console.WriteLine($"Text size = {text.Size}");
        // Parent size is now invalidated and recomputed (spooky "getter changes value").
        Console.WriteLine($"Parent size = {text.Parent.Size}");
    });
}

A similar test case with CompositeDrawable's autosize implementation is:

[Test]
public void TestCaseAutoSizeInvalidationPropagation()
{
    Box box = null;
    AddStep("create test", () =>
    {
        Child = new Container
        {
            AutoSizeAxes = Axes.Both,
            Child = new Container
            {
                AutoSizeAxes = Axes.Both,
                Child = box = new Box
                {
                    Colour = Colour4.Green,
                    Size = new Vector2(100)
                }
            },
        };
    });
    AddStep("modify box size", () =>
    {
        box.Size = new Vector2(200);
        // Grandparent size is stale.
        Console.WriteLine($"Grandparent size = {box.Parent.Parent.Size}");
        // Parent size is updated.
        Console.WriteLine($"Parent size = {box.Parent.Size}");
        // Grandparent size is updated now
        Console.WriteLine($"Grandparent size again = {box.Parent.Parent.Size}");
    });
}

And a similar fix for CompositeDrawable is adding something like this in InvalidateChildrenSizeDependencies

 if ((axes & AutoSizeAxes) != 0)
     Invalidate(Invalidation.DrawSize);

I haven't thought much about this, but is there a reason to not do something like this? @smoogipoo

[smoogipoo]

Yeah, I also just saw the SpriteText issue while looking into it.

I haven't thought much about this, but is there a reason to not do something like this?

Mostly performance concerns. Getting it working without severely impacting performance is a little more involved. I've already begun to work on it in https://github.com/smoogipoo/osu-framework/tree/fix-nested-autosize. Feel free to continue on it if you get to it before me - only the Add test case is failing.

Otherwise, I'll probably finish it tomorrow :P

Also note that the above branch/suggestions won't close this issue - it won't fix for example a carefully-crafted FillFlowContainer or TextFlowContainer example. Basically any component that updates layout in Update() or UpdateAfterChildren() runs into the same issue. So I was hoping this issue would be resolvable in a more general way - I think you had a PR with validation dependencies at some point which I'd like to look into a bit more.

[ekrctb]

I'm not understanding the situation fully, and I'll just wait for your work. One thing is that perhaps the performance concern should be tested somehow. When tests are there I can tweak the code to experiment.