ppy / osu-framework

A game framework written with osu! in mind.
MIT License
1.64k stars 412 forks source link

Autosize invalidation optimization of `BypassAutosizeAxes` is not correct when child is rotated #4001

Open ekrctb opened 3 years ago

ekrctb commented 3 years ago

This is introduced by #3327. I was reading that PR and I came up with a corner test.

        [Test]
        public void TestBypassAutoSizeAxesAndRotatation()
        {
            Drawable container = null, rotatedBox = null;
            AddStep("create test", () => Child = container = new Container
            {
                AutoSizeAxes = Axes.Y,
                Width = 100,
                Position = new Vector2(100),
                Children = new Drawable[] {
                    new Box // Not relevant to the issue. Just for a visual testing.
                    {
                        RelativeSizeAxes = Axes.Both,
                        Colour = Colour4.White,
                    },
                    rotatedBox = new Box {
                        Size = new Vector2(50, 50),
                        Colour = Colour4.Blue,
                        Rotation = 45,
                        BypassAutoSizeAxes = Axes.X,
                    }
                }
            });
            AddStep("resize rotated box", () => rotatedBox.Width = 100);
            AddAssert("container size is changed", () => Precision.AlmostEquals(container.Height, 150 * Math.Sqrt(2) / 2));
        }
smoogipoo commented 3 years ago

Nice find. This only breaks with self-rotation, right? Maybe an appropriate solution is to check Rotation == 0 before going into the fast path?

ekrctb commented 3 years ago

The issue also happens with Shear. In general, if the transformation matrix has a non-zero entry in the specific coordinates then a child width change affects parent height. A general way to fix the issue might be checking if the requiredParentSizeToFitBacking is actually changed by the property (such as Width) change. And then we can invalidate only the actually changed coordinates.

It is not directly related to the issue but I think we can make the fast-path optimization less ad-hoc by separating Invalidation.RequiredParentSizeToFit to X and Y sizes.

smoogipoo commented 3 years ago

I'm not against such splitting - I've thought similar with splitting autosize up in the past and would still like to do that at some point.