Skip to content
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

Allow to do DrawNode invalidation on cached BufferedContainer without redrawing the framebuffer #6404

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,8 @@ public TestSceneCachedBufferedContainer()
"uncached with parent scale&fade",
"cached with parent scale&fade",
"cached with no redraw on parent scale&fade",
"cached with drawnode invalidation",
"cached with force redraw",
};

var boxes = new List<ContainingBox>();
Expand All @@ -48,9 +50,14 @@ public TestSceneCachedBufferedContainer()
Text = labels[i],
Font = new FontUsage(size: 20),
},
box = new ContainingBox(i >= 6, i >= 8)
box = new ContainingBox(i >= 6 && i <= 10, i >= 8 && i <= 10)
{
Child = new CountingBox(i == 2 || i == 3, i == 4 || i == 5, cached: i % 2 == 1 || i == 10)
Child = new CountingBox(
rotating: i == 2 || i == 3,
moving: i == 4 || i == 5,
invalidate: i == 11,
forceRedraw: i == 12,
cached: i % 2 == 1 || i == 10 || i == 12)
{
RedrawOnScale = i != 10
},
Expand All @@ -67,7 +74,8 @@ public TestSceneCachedBufferedContainer()
AddAssert("even box counts equal", () =>
boxes[0].Count == boxes[2].Count &&
boxes[2].Count == boxes[4].Count &&
boxes[4].Count == boxes[6].Count);
boxes[4].Count == boxes[6].Count &&
boxes[6].Count == boxes[8].Count);

// ensure cached is never updating children.
AddAssert("box 1 count is 1", () => boxes[1].Count == 1);
Expand All @@ -86,6 +94,12 @@ public TestSceneCachedBufferedContainer()
AddAssert("box 7 count equals box 8 count", () => boxes[7].Count == boxes[8].Count);

AddAssert("box 10 count is 1", () => boxes[10].Count == 1);

// ensure drawnode invalidation doesn't invalidate cache.
AddAssert("box 11 count is 1", () => boxes[11].Count == 1);

// ensure force redraw always invalidates cache.
AddAssert("box 12 count equals box 0 count", () => boxes[12].Count == boxes[0].Count);
}

private partial class ContainingBox : Container<CountingBox>
Expand Down Expand Up @@ -117,13 +131,17 @@ private partial class CountingBox : BufferedContainer

private readonly bool rotating;
private readonly bool moving;
private readonly bool invalidate;
private readonly bool forceRedraw;
private readonly SpriteText count;

public CountingBox(bool rotating = false, bool moving = false, bool cached = false)
public CountingBox(bool rotating = false, bool moving = false, bool invalidate = false, bool forceRedraw = false, bool cached = false)
: base(cachedFrameBuffer: cached)
{
this.rotating = rotating;
this.moving = moving;
this.invalidate = invalidate;
this.forceRedraw = forceRedraw;
RelativeSizeAxes = Axes.Both;
Origin = Anchor.Centre;
Anchor = Anchor.Centre;
Expand Down Expand Up @@ -153,6 +171,16 @@ protected override void Update()
{
base.Update();

if (invalidate)
{
Invalidate(Invalidation.DrawNode);
}

if (forceRedraw)
{
ForceRedraw();
}

if (RequiresChildrenUpdate)
{
Count++;
Expand Down
18 changes: 13 additions & 5 deletions osu.Framework/Graphics/Containers/BufferedContainer.cs
Original file line number Diff line number Diff line change
Expand Up @@ -117,7 +117,7 @@ public ColourInfo EffectColour
return;

effectColour = value;
Invalidate(Invalidation.DrawNode);
ForceRedraw();
}
}

Expand All @@ -137,7 +137,7 @@ public BlendingParameters EffectBlending
return;

effectBlending = value;
Invalidate(Invalidation.DrawNode);
ForceRedraw();
}
}

Expand All @@ -156,7 +156,7 @@ public EffectPlacement EffectPlacement
return;

effectPlacement = value;
Invalidate(Invalidation.DrawNode);
ForceRedraw();
}
}

Expand Down Expand Up @@ -223,7 +223,11 @@ public bool RedrawOnScale
/// Forces a redraw of the framebuffer before it is blitted the next time.
/// Only relevant if <see cref="UsingCachedFrameBuffer"/> is true.
/// </summary>
public void ForceRedraw() => Invalidate(Invalidation.DrawNode);
public void ForceRedraw()
{
++updateVersion;
Invalidate(Invalidation.DrawNode);
}

/// <summary>
/// In order to signal the draw thread to re-draw the buffered container we version it.
Expand Down Expand Up @@ -293,7 +297,11 @@ protected override bool OnInvalidate(Invalidation invalidation, InvalidationSour

if ((invalidation & Invalidation.DrawNode) > 0)
{
++updateVersion;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additionally, we'll need to make sure this doesn't break any existing cases (or figure out how to fix those). I believe this currently means changing Padding or adding a child will redraw?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added tests for Padding and adding new internal child, and no, they don't cause redraw. However, it doesn't cause redraw in master (bc83409) either.
I don't know if I should push those tests since they are failing, but it's not caused by this change, so for now - you can find them here: Uncomfy/osu-framework@buffered_container_explicit_framebuffer_redraw...buffered_container_extra_tests

if (source == InvalidationSource.Child)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this will only work for immediate children, because Child invalidations currently don't propagate all the way to the root.

{
++updateVersion;
}

result = true;
}

Expand Down
Loading