New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clean up BufferedContainer and fix alignment issues #1546

Merged
merged 13 commits into from May 11, 2018

Conversation

2 participants
@smoogipoo
Contributor

smoogipoo commented May 10, 2018

Fixes ppy/osu#2268

At this point I've spent about 1.5 days trying to nail down the exact frame-by-frame issue, to no avail, so I'm going to PR this as it is a sane change.

The issue that sparked this is ppy/osu#2268 and similar issues happening in BreakOverlay:
screen shot 2018-05-09 at 12 44 35 pm

It seems that the issue is a mix of invalidation + frame timings + values of AddChildDrawNodes / RequiresChildrenUpdate, causing what I imagine: the BufferedContainerDrawNode receives an updated ScreenSpaceDrawRectangle which it uses for the ortho projection, but the child of the BufferedContainerDrawNode doesn't get updated.

Went through and sanity checked most of BufferedContainer - making it use the common "shared data" paradigm that's used for other drawnodes, adjusting draw/child update versions to start at -1 when no draw/update has happened, cleaning up the mess that was overriding GenerateDrawNodeSubtree, etc.

730ef7d - Fix child drawnodes generation

This is the change that fixed the aforementioned issues. It:

  1. Removes the extra version array. This by itself doesn't fix the issues.
  2. Shares the same conditional to check if the drawnode will be redrawn. This by itself doesn't fix the issues.
  3. Moves the setting of AddChildDrawNodes to ApplyDrawNode. This seems to be what fixes the issues.

In regards to (3) - it's a sane change regardless of it fixing the issue, as the only time AddChildDrawNodes needs to be evaluated is when the DrawNode is invalidated (i.e. ApplyDrawNode is called), in all other times it should be false.

Replication

I replicated this in a testcase, though unreliably, through the following code. It'll eventually replicate if you keep reloading the testcase.

public class TestCaseScratch : FrameworkTestCase
{
    public TestCaseScratch()
    {
        Child = new TestBlurredIcon
        {
            Anchor = Anchor.Centre,
            Origin = Anchor.Centre
        };
    }

    private class TestBlurredIcon : BufferedContainer
    {
        public TestBlurredIcon()
        {
            CacheDrawnFrameBuffer = true;

            BlurSigma = new Vector2(40);
            Size = new Vector2(50) + BlurSigma * 2.5f;

            EffectColour = Colour.MultiplyAlpha(4);
            Colour = Color4.LightSkyBlue;

            X = -75; // Important!

            Child = new CircularContainer
            {
                Anchor = Anchor.Centre,
                Origin = Anchor.Centre,
                Size = new Vector2(50),
                Masking = true,
                Child = new Box { RelativeSizeAxes = Axes.Both }
            };
        }

        private long frame;

        protected override void Update()
        {
            base.Update();

            frame++;

            // Technically any value > 1 should work here, but the issue occurred most readily with 4 for me
            if (frame == 4)
                X = 0;
        }
    }
}

@smoogipoo smoogipoo added the drawable label May 10, 2018

@smoogipoo smoogipoo added this to the May 2018 milestone May 10, 2018

@smoogipoo smoogipoo requested review from peppy and Tom94 May 10, 2018

@peppy

peppy approved these changes May 10, 2018

looks good here 👍

@peppy peppy changed the title from Cleanup + fix up BufferedContainer issues to Clean up BufferedContainer and fix alignment issues May 10, 2018

peppy added some commits May 10, 2018

@peppy peppy merged commit c4ea817 into ppy:master May 11, 2018

1 check passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment