Merge pull request #4419 from nekodex/fix-duplicate-bg-sprites

Fix UpdateableBeatmapBackgroundSprite not disposing of previously loaded sprites
This commit is contained in:
Dean Herbert
2019-03-08 12:34:56 +09:00
committed by GitHub
2 changed files with 54 additions and 34 deletions

View File

@ -16,7 +16,7 @@ namespace osu.Game.Tests.Visual
{ {
public class TestCaseUpdateableBeatmapBackgroundSprite : OsuTestCase public class TestCaseUpdateableBeatmapBackgroundSprite : OsuTestCase
{ {
private UpdateableBeatmapBackgroundSprite backgroundSprite; private TestUpdateableBeatmapBackgroundSprite backgroundSprite;
[Resolved] [Resolved]
private BeatmapManager beatmaps { get; set; } private BeatmapManager beatmaps { get; set; }
@ -28,30 +28,36 @@ namespace osu.Game.Tests.Visual
var imported = ImportBeatmapTest.LoadOszIntoOsu(osu); var imported = ImportBeatmapTest.LoadOszIntoOsu(osu);
Child = backgroundSprite = new UpdateableBeatmapBackgroundSprite { RelativeSizeAxes = Axes.Both }; Child = backgroundSprite = new TestUpdateableBeatmapBackgroundSprite { RelativeSizeAxes = Axes.Both };
backgroundSprite.Beatmap.BindTo(beatmapBindable); backgroundSprite.Beatmap.BindTo(beatmapBindable);
var req = new GetBeatmapSetRequest(1); var req = new GetBeatmapSetRequest(1);
api.Queue(req); api.Queue(req);
AddStep("null", () => beatmapBindable.Value = null); AddStep("load null beatmap", () => beatmapBindable.Value = null);
AddUntilStep(() => backgroundSprite.ChildCount == 1, "wait for cleanup...");
AddStep("imported", () => beatmapBindable.Value = imported.Beatmaps.First()); AddStep("load imported beatmap", () => beatmapBindable.Value = imported.Beatmaps.First());
AddUntilStep(() => backgroundSprite.ChildCount == 1, "wait for cleanup...");
if (api.IsLoggedIn) if (api.IsLoggedIn)
{ {
AddUntilStep(() => req.Result != null, "wait for api response"); AddUntilStep(() => req.Result != null, "wait for api response");
AddStep("load online beatmap", () => beatmapBindable.Value = new BeatmapInfo
AddStep("online", () => beatmapBindable.Value = new BeatmapInfo
{ {
BeatmapSet = req.Result?.ToBeatmapSet(rulesets) BeatmapSet = req.Result?.ToBeatmapSet(rulesets)
}); });
AddUntilStep(() => backgroundSprite.ChildCount == 1, "wait for cleanup...");
} }
else else
{ {
AddStep("online (login first)", () => { }); AddStep("online (login first)", () => { });
} }
} }
private class TestUpdateableBeatmapBackgroundSprite : UpdateableBeatmapBackgroundSprite
{
public int ChildCount => InternalChildren.Count;
}
} }
} }

View File

@ -9,7 +9,7 @@ using osu.Framework.Graphics.Containers;
namespace osu.Game.Beatmaps.Drawables namespace osu.Game.Beatmaps.Drawables
{ {
/// <summary> /// <summary>
/// Display a baetmap background from a local source, but fallback to online source if not available. /// Display a beatmap background from a local source, but fallback to online source if not available.
/// </summary> /// </summary>
public class UpdateableBeatmapBackgroundSprite : ModelBackedDrawable<BeatmapInfo> public class UpdateableBeatmapBackgroundSprite : ModelBackedDrawable<BeatmapInfo>
{ {
@ -26,37 +26,51 @@ namespace osu.Game.Beatmaps.Drawables
this.beatmapSetCoverType = beatmapSetCoverType; this.beatmapSetCoverType = beatmapSetCoverType;
} }
protected override Drawable CreateDrawable(BeatmapInfo model) private BeatmapInfo lastModel;
protected override DelayedLoadWrapper CreateDelayedLoadWrapper(Drawable content, double timeBeforeLoad)
{ {
return new DelayedLoadUnloadWrapper(() => return new DelayedLoadUnloadWrapper(() =>
{ {
Drawable drawable; // If DelayedLoadUnloadWrapper is attempting to RELOAD the same content (Beatmap), that means that it was
// previously UNLOADED and thus its children have been disposed of, so we need to recreate them here.
if (lastModel == Beatmap.Value && Beatmap.Value != null)
return CreateDrawable(Beatmap.Value);
var localBeatmap = beatmaps.GetWorkingBeatmap(model); // If the model has changed since the previous unload (or if there was no load), then we can safely use the given content
lastModel = Beatmap.Value;
if (model?.BeatmapSet?.OnlineInfo != null) return content;
drawable = new BeatmapSetCover(model.BeatmapSet, beatmapSetCoverType); }, timeBeforeLoad, 10000);
else if (localBeatmap.BeatmapInfo.ID != 0)
{
// Fall back to local background if one exists
drawable = new BeatmapBackgroundSprite(localBeatmap);
}
else
{
// Use the default background if somehow an online set does not exist and we don't have a local copy.
drawable = new BeatmapBackgroundSprite(beatmaps.DefaultBeatmap);
}
drawable.RelativeSizeAxes = Axes.Both;
drawable.Anchor = Anchor.Centre;
drawable.Origin = Anchor.Centre;
drawable.FillMode = FillMode.Fill;
drawable.OnLoadComplete = d => d.FadeInFromZero(400);
return drawable;
}, 500, 10000);
} }
protected override double FadeDuration => 0; protected override Drawable CreateDrawable(BeatmapInfo model)
{
Drawable drawable;
var localBeatmap = beatmaps.GetWorkingBeatmap(model);
if (model?.BeatmapSet?.OnlineInfo != null)
{
drawable = new BeatmapSetCover(model.BeatmapSet, beatmapSetCoverType);
}
else if (localBeatmap.BeatmapInfo.ID != 0)
{
// Fall back to local background if one exists
drawable = new BeatmapBackgroundSprite(localBeatmap);
}
else
{
// Use the default background if somehow an online set does not exist and we don't have a local copy.
drawable = new BeatmapBackgroundSprite(beatmaps.DefaultBeatmap);
}
drawable.RelativeSizeAxes = Axes.Both;
drawable.Anchor = Anchor.Centre;
drawable.Origin = Anchor.Centre;
drawable.FillMode = FillMode.Fill;
drawable.OnLoadComplete = d => d.FadeInFromZero(400);
return drawable;
}
} }
} }