Merge branch 'master' into legacy-skin-resource-improvements

osu.Game.Rulesets.Catch.Tests/TestSceneCatchPlayerLegacySkin.cs

@@ -1,8 +1,16 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System.Linq;
 using NUnit.Framework;
+using osu.Framework.Extensions.IEnumerableExtensions;
+using osu.Framework.Graphics.Containers;
+using osu.Framework.Screens;
+using osu.Framework.Testing;
+using osu.Game.Screens.Play.HUD;
+using osu.Game.Skinning;
 using osu.Game.Tests.Visual;
+using osuTK;
 
 namespace osu.Game.Rulesets.Catch.Tests
 {
@@ -10,5 +18,22 @@ namespace osu.Game.Rulesets.Catch.Tests
     public class TestSceneCatchPlayerLegacySkin : LegacySkinPlayerTestScene
     {
         protected override Ruleset CreatePlayerRuleset() => new CatchRuleset();
+
+        [Test]
+        public void TestLegacyHUDComboCounterHidden([Values] bool withModifiedSkin)
+        {
+            if (withModifiedSkin)
+            {
+                AddStep("change component scale", () => Player.ChildrenOfType<LegacyScoreCounter>().First().Scale = new Vector2(2f));
+                AddStep("update target", () => Player.ChildrenOfType<SkinnableTargetContainer>().ForEach(LegacySkin.UpdateDrawableTarget));
+                AddStep("exit player", () => Player.Exit());
+                CreateTest(null);
+            }
+
+            AddAssert("legacy HUD combo counter hidden", () =>
+            {
+                return Player.ChildrenOfType<LegacyComboCounter>().All(c => c.ChildrenOfType<Container>().Single().Alpha == 0f);
+            });
+        }
     }
 }

osu.Game.Rulesets.Catch/Skinning/Legacy/CatchLegacySkinTransformer.cs

@@ -1,8 +1,10 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
+using System.Linq;
 using osu.Framework.Bindables;
 using osu.Framework.Graphics;
+using osu.Game.Screens.Play.HUD;
 using osu.Game.Skinning;
 using osuTK.Graphics;
 
@@ -22,16 +24,22 @@ namespace osu.Game.Rulesets.Catch.Skinning.Legacy
 
         public override Drawable GetDrawableComponent(ISkinComponent component)
         {
-            if (component is HUDSkinComponent hudComponent)
+            if (component is SkinnableTargetComponent targetComponent)
             {
-                switch (hudComponent.Component)
+                switch (targetComponent.Target)
                 {
-                    case HUDSkinComponents.ComboCounter:
+                    case SkinnableTarget.MainHUDComponents:
-                        // catch may provide its own combo counter; hide the default.
+                        var components = Source.GetDrawableComponent(component) as SkinnableTargetComponentsContainer;
-                        if (providesComboCounter)
-                            return Drawable.Empty();
 
-                        break;
+                        if (providesComboCounter && components != null)
+                        {
+                            // catch may provide its own combo counter; hide the default.
+                            // todo: this should be done in an elegant way per ruleset, defining which HUD skin components should be displayed.
+                            foreach (var legacyComboCounter in components.OfType<LegacyComboCounter>())
+                                legacyComboCounter.HiddenByRulesetImplementation = false;
+                        }
+
+                        return components;
                 }
             }
 

osu.Game.Tests/Visual/Editing/TestSceneEditorBeatmapCreation.cs

@@ -24,13 +24,13 @@ namespace osu.Game.Tests.Visual.Editing
 
         protected override bool EditorComponentsReady => Editor.ChildrenOfType<SetupScreen>().SingleOrDefault()?.IsLoaded == true;
 
+        protected override bool IsolateSavingFromDatabase => false;
+
         [Resolved]
         private BeatmapManager beatmapManager { get; set; }
 
         public override void SetUpSteps()
         {
-            AddStep("set dummy", () => Beatmap.Value = new DummyWorkingBeatmap(Audio, null));
-
             base.SetUpSteps();
 
             // if we save a beatmap with a hash collision, things fall over.
@@ -38,6 +38,12 @@ namespace osu.Game.Tests.Visual.Editing
             AddStep("make new beatmap unique", () => EditorBeatmap.Metadata.Title = Guid.NewGuid().ToString());
         }
 
+        protected override void LoadEditor()
+        {
+            Beatmap.Value = new DummyWorkingBeatmap(Audio, null);
+            base.LoadEditor();
+        }
+
         [Test]
         public void TestCreateNewBeatmap()
         {

osu.Game/Beatmaps/BeatmapManager.cs

@@ -3,7 +3,6 @@
 
 using System;
 using System.Collections.Generic;
-using System.Diagnostics;
 using System.IO;
 using System.Linq;
 using System.Linq.Expressions;
@@ -243,12 +242,10 @@ namespace osu.Game.Beatmaps
         /// <param name="info">The <see cref="BeatmapInfo"/> to save the content against. The file referenced by <see cref="BeatmapInfo.Path"/> will be replaced.</param>
         /// <param name="beatmapContent">The <see cref="IBeatmap"/> content to write.</param>
         /// <param name="beatmapSkin">The beatmap <see cref="ISkin"/> content to write, null if to be omitted.</param>
-        public void Save(BeatmapInfo info, IBeatmap beatmapContent, ISkin beatmapSkin = null)
+        public virtual void Save(BeatmapInfo info, IBeatmap beatmapContent, ISkin beatmapSkin = null)
         {
             var setInfo = info.BeatmapSet;
 
-            Debug.Assert(setInfo.Files != null);
-
             using (var stream = new MemoryStream())
             {
                 using (var sw = new StreamWriter(stream, Encoding.UTF8, 1024, true))
@@ -285,25 +282,17 @@ namespace osu.Game.Beatmaps
         /// Retrieve a <see cref="WorkingBeatmap"/> instance for the provided <see cref="BeatmapInfo"/>
         /// </summary>
         /// <param name="beatmapInfo">The beatmap to lookup.</param>
-        /// <param name="previous">The currently loaded <see cref="WorkingBeatmap"/>. Allows for optimisation where elements are shared with the new beatmap. May be returned if beatmapInfo requested matches</param>
         /// <returns>A <see cref="WorkingBeatmap"/> instance correlating to the provided <see cref="BeatmapInfo"/>.</returns>
-        public WorkingBeatmap GetWorkingBeatmap(BeatmapInfo beatmapInfo, WorkingBeatmap previous = null)
+        public virtual WorkingBeatmap GetWorkingBeatmap(BeatmapInfo beatmapInfo)
         {
-            if (beatmapInfo?.ID > 0 && previous != null && previous.BeatmapInfo?.ID == beatmapInfo.ID)
+            // if there are no files, presume the full beatmap info has not yet been fetched from the database.
-                return previous;
+            if (beatmapInfo?.BeatmapSet?.Files.Count == 0)
-
-            if (beatmapInfo?.BeatmapSet == null || beatmapInfo == DefaultBeatmap?.BeatmapInfo)
-                return DefaultBeatmap;
-
-            // force a re-query if files are not in a state which looks like the model has
-            // full database information present.
-            if (beatmapInfo.BeatmapSet.Files == null || beatmapInfo.BeatmapSet.Files.Count == 0)
             {
-                var info = beatmapInfo;
+                int lookupId = beatmapInfo.ID;
-                beatmapInfo = QueryBeatmap(b => b.ID == info.ID);
+                beatmapInfo = QueryBeatmap(b => b.ID == lookupId);
             }
 
-            if (beatmapInfo == null)
+            if (beatmapInfo?.BeatmapSet == null)
                 return DefaultBeatmap;
 
             lock (workingCache)

osu.Game/Beatmaps/BeatmapSetInfo.cs

@@ -5,6 +5,7 @@ using System;
 using System.Collections.Generic;
 using System.ComponentModel.DataAnnotations.Schema;
 using System.Linq;
+using JetBrains.Annotations;
 using osu.Framework.Testing;
 using osu.Game.Database;
 
@@ -31,6 +32,9 @@ namespace osu.Game.Beatmaps
 
         public List<BeatmapInfo> Beatmaps { get; set; }
 
+        [NotNull]
+        public List<BeatmapSetFileInfo> Files { get; set; } = new List<BeatmapSetFileInfo>();
+
         [NotMapped]
         public BeatmapSetOnlineInfo OnlineInfo { get; set; }
 
@@ -57,16 +61,14 @@ namespace osu.Game.Beatmaps
 
         public string Hash { get; set; }
 
-        public string StoryboardFile => Files?.Find(f => f.Filename.EndsWith(".osb", StringComparison.OrdinalIgnoreCase))?.Filename;
+        public string StoryboardFile => Files.Find(f => f.Filename.EndsWith(".osb", StringComparison.OrdinalIgnoreCase))?.Filename;
 
         /// <summary>
         /// Returns the storage path for the file in this beatmapset with the given filename, if any exists, otherwise null.
         /// The path returned is relative to the user file storage.
         /// </summary>
         /// <param name="filename">The name of the file to get the storage path of.</param>
-        public string GetPathForFile(string filename) => Files?.SingleOrDefault(f => string.Equals(f.Filename, filename, StringComparison.OrdinalIgnoreCase))?.FileInfo.StoragePath;
+        public string GetPathForFile(string filename) => Files.SingleOrDefault(f => string.Equals(f.Filename, filename, StringComparison.OrdinalIgnoreCase))?.FileInfo.StoragePath;
-
-        public List<BeatmapSetFileInfo> Files { get; set; }
 
         public override string ToString() => Metadata?.ToString() ?? base.ToString();
 

osu.Game/Overlays/MusicController.cs

@@ -252,7 +252,7 @@ namespace osu.Game.Overlays
 
             if (playable != null)
             {
-                changeBeatmap(beatmaps.GetWorkingBeatmap(playable.Beatmaps.First(), beatmap.Value));
+                changeBeatmap(beatmaps.GetWorkingBeatmap(playable.Beatmaps.First()));
                 restartTrack();
                 return PreviousTrackResult.Previous;
             }
@@ -283,7 +283,7 @@ namespace osu.Game.Overlays
 
             if (playable != null)
             {
-                changeBeatmap(beatmaps.GetWorkingBeatmap(playable.Beatmaps.First(), beatmap.Value));
+                changeBeatmap(beatmaps.GetWorkingBeatmap(playable.Beatmaps.First()));
                 restartTrack();
                 return true;
             }

osu.Game/Screens/Play/HUD/LegacyComboCounter.cs

@@ -17,7 +17,7 @@ namespace osu.Game.Screens.Play.HUD
     /// </summary>
     public class LegacyComboCounter : CompositeDrawable, ISkinnableDrawable
     {
-        public Bindable<int> Current { get; } = new BindableInt { MinValue = 0, };
+        public Bindable<int> Current { get; } = new BindableInt { MinValue = 0 };
 
         private uint scheduledPopOutCurrentId;
 
@@ -32,9 +32,9 @@ namespace osu.Game.Screens.Play.HUD
         /// </summary>
         private const double rolling_duration = 20;
 
-        private Drawable popOutCount;
+        private readonly Drawable popOutCount;
 
-        private Drawable displayedCountSpriteText;
+        private readonly Drawable displayedCountSpriteText;
 
         private int previousValue;
 
@@ -45,6 +45,20 @@ namespace osu.Game.Screens.Play.HUD
         [Resolved]
         private ISkinSource skin { get; set; }
 
+        private readonly Container counterContainer;
+
+        /// <summary>
+        /// Hides the combo counter internally without affecting its <see cref="SkinnableInfo"/>.
+        /// </summary>
+        /// <remarks>
+        /// This is used for rulesets that provide their own combo counter and don't want this HUD one to be visible,
+        /// without potentially affecting the user's selected skin.
+        /// </remarks>
+        public bool HiddenByRulesetImplementation
+        {
+            set => counterContainer.Alpha = value ? 1 : 0;
+        }
+
         public LegacyComboCounter()
         {
             AutoSizeAxes = Axes.Both;
@@ -55,6 +69,25 @@ namespace osu.Game.Screens.Play.HUD
             Margin = new MarginPadding(10);
 
             Scale = new Vector2(1.2f);
+
+            InternalChild = counterContainer = new Container
+            {
+                AutoSizeAxes = Axes.Both,
+                AlwaysPresent = true,
+                Children = new[]
+                {
+                    popOutCount = new LegacySpriteText(LegacyFont.Combo)
+                    {
+                        Alpha = 0,
+                        Margin = new MarginPadding(0.05f),
+                        Blending = BlendingParameters.Additive,
+                    },
+                    displayedCountSpriteText = new LegacySpriteText(LegacyFont.Combo)
+                    {
+                        Alpha = 0,
+                    },
+                }
+            };
         }
 
         /// <summary>
@@ -82,20 +115,6 @@ namespace osu.Game.Screens.Play.HUD
         [BackgroundDependencyLoader]
         private void load(ScoreProcessor scoreProcessor)
         {
-            InternalChildren = new[]
-            {
-                popOutCount = new LegacySpriteText(LegacyFont.Combo)
-                {
-                    Alpha = 0,
-                    Margin = new MarginPadding(0.05f),
-                    Blending = BlendingParameters.Additive,
-                },
-                displayedCountSpriteText = new LegacySpriteText(LegacyFont.Combo)
-                {
-                    Alpha = 0,
-                },
-            };
-
             Current.BindTo(scoreProcessor.Combo);
         }
 
@@ -105,10 +124,12 @@ namespace osu.Game.Screens.Play.HUD
 
             ((IHasText)displayedCountSpriteText).Text = formatCount(Current.Value);
 
+            counterContainer.Anchor = Anchor;
+            counterContainer.Origin = Origin;
             displayedCountSpriteText.Anchor = Anchor;
             displayedCountSpriteText.Origin = Origin;
-            popOutCount.Origin = Origin;
             popOutCount.Anchor = Anchor;
+            popOutCount.Origin = Origin;
 
             Current.BindValueChanged(combo => updateCount(combo.NewValue == 0), true);
         }

osu.Game/Screens/Select/SongSelect.cs

@@ -505,12 +505,13 @@ namespace osu.Game.Screens.Select
                 {
                     Logger.Log($"beatmap changed from \"{Beatmap.Value.BeatmapInfo}\" to \"{beatmap}\"");
 
-                    WorkingBeatmap previous = Beatmap.Value;
+                    int? lastSetID = Beatmap.Value?.BeatmapInfo.BeatmapSetInfoID;
-                    Beatmap.Value = beatmaps.GetWorkingBeatmap(beatmap, previous);
+
+                    Beatmap.Value = beatmaps.GetWorkingBeatmap(beatmap);
 
                     if (beatmap != null)
                     {
-                        if (beatmap.BeatmapSetInfoID == previous?.BeatmapInfo.BeatmapSetInfoID)
+                        if (beatmap.BeatmapSetInfoID == lastSetID)
                             sampleChangeDifficulty.Play();
                         else
                             sampleChangeBeatmap.Play();

osu.Game/Skinning/LegacySkin.cs

@@ -333,7 +333,6 @@ namespace osu.Game.Skinning
                     switch (target.Target)
                     {
                         case SkinnableTarget.MainHUDComponents:
-
                             var skinnableTargetWrapper = new SkinnableTargetComponentsContainer(container =>
                             {
                                 var score = container.OfType<LegacyScoreCounter>().FirstOrDefault();

osu.Game/Storyboards/Drawables/DrawableStoryboardVideo.cs

@@ -29,7 +29,7 @@ namespace osu.Game.Storyboards.Drawables
         [BackgroundDependencyLoader(true)]
         private void load(IBindable<WorkingBeatmap> beatmap, TextureStore textureStore)
         {
-            var path = beatmap.Value.BeatmapSetInfo?.Files?.Find(f => f.Filename.Equals(Video.Path, StringComparison.OrdinalIgnoreCase))?.FileInfo.StoragePath;
+            var path = beatmap.Value.BeatmapSetInfo?.Files.Find(f => f.Filename.Equals(Video.Path, StringComparison.OrdinalIgnoreCase))?.FileInfo.StoragePath;
 
             if (path == null)
                 return;

osu.Game/Storyboards/Storyboard.cs

@@ -91,7 +91,7 @@ namespace osu.Game.Storyboards
         public Drawable CreateSpriteFromResourcePath(string path, TextureStore textureStore)
         {
             Drawable drawable = null;
-            var storyboardPath = BeatmapInfo.BeatmapSet?.Files?.Find(f => f.Filename.Equals(path, StringComparison.OrdinalIgnoreCase))?.FileInfo.StoragePath;
+            var storyboardPath = BeatmapInfo.BeatmapSet?.Files.Find(f => f.Filename.Equals(path, StringComparison.OrdinalIgnoreCase))?.FileInfo.StoragePath;
 
             if (storyboardPath != null)
                 drawable = new Sprite { Texture = textureStore.Get(storyboardPath) };

osu.Game/Tests/Beatmaps/TestBeatmap.cs

@@ -29,7 +29,6 @@ namespace osu.Game.Tests.Beatmaps
             BeatmapInfo.Ruleset = ruleset;
             BeatmapInfo.RulesetID = ruleset.ID ?? 0;
             BeatmapInfo.BeatmapSet.Metadata = BeatmapInfo.Metadata;
-            BeatmapInfo.BeatmapSet.Files = new List<BeatmapSetFileInfo>();
             BeatmapInfo.BeatmapSet.Beatmaps = new List<BeatmapInfo> { BeatmapInfo };
             BeatmapInfo.Length = 75000;
             BeatmapInfo.OnlineInfo = new BeatmapOnlineInfo();

osu.Game/Tests/Visual/EditorTestScene.cs

@@ -4,11 +4,19 @@
 using System.Linq;
 using JetBrains.Annotations;
 using osu.Framework.Allocation;
+using osu.Framework.Audio;
+using osu.Framework.IO.Stores;
+using osu.Framework.Platform;
 using osu.Framework.Testing;
+using osu.Game.Beatmaps;
+using osu.Game.Database;
+using osu.Game.IO.Archives;
+using osu.Game.Online.API;
 using osu.Game.Rulesets;
 using osu.Game.Rulesets.Edit;
 using osu.Game.Screens.Edit;
 using osu.Game.Screens.Edit.Compose.Components.Timeline;
+using osu.Game.Skinning;
 
 namespace osu.Game.Tests.Visual
 {
@@ -20,10 +28,20 @@ namespace osu.Game.Tests.Visual
 
         protected EditorClock EditorClock { get; private set; }
 
+        /// <summary>
+        /// Whether any saves performed by the editor should be isolate (and not persist) to the underlying <see cref="BeatmapManager"/>.
+        /// </summary>
+        protected virtual bool IsolateSavingFromDatabase => true;
+
         [BackgroundDependencyLoader]
-        private void load()
+        private void load(GameHost host, AudioManager audio, RulesetStore rulesets)
         {
-            Beatmap.Value = CreateWorkingBeatmap(Ruleset.Value);
+            var working = CreateWorkingBeatmap(Ruleset.Value);
+
+            Beatmap.Value = working;
+
+            if (IsolateSavingFromDatabase)
+                Dependencies.CacheAs<BeatmapManager>(new TestBeatmapManager(LocalStorage, ContextFactory, rulesets, null, audio, Resources, host, Beatmap.Default, working));
         }
 
         protected virtual bool EditorComponentsReady => Editor.ChildrenOfType<HitObjectComposer>().FirstOrDefault()?.IsLoaded == true
@@ -33,12 +51,17 @@ namespace osu.Game.Tests.Visual
         {
             base.SetUpSteps();
 
-            AddStep("load editor", () => LoadScreen(Editor = CreateEditor()));
+            AddStep("load editor", LoadEditor);
             AddUntilStep("wait for editor to load", () => EditorComponentsReady);
             AddStep("get beatmap", () => EditorBeatmap = Editor.ChildrenOfType<EditorBeatmap>().Single());
             AddStep("get clock", () => EditorClock = Editor.ChildrenOfType<EditorClock>().Single());
         }
 
+        protected virtual void LoadEditor()
+        {
+            LoadScreen(Editor = CreateEditor());
+        }
+
         /// <summary>
         /// Creates the ruleset for providing a corresponding beatmap to load the editor on.
         /// </summary>
@@ -65,5 +88,27 @@ namespace osu.Game.Tests.Visual
 
             public new bool HasUnsavedChanges => base.HasUnsavedChanges;
         }
+
+        private class TestBeatmapManager : BeatmapManager
+        {
+            private readonly WorkingBeatmap testBeatmap;
+
+            public TestBeatmapManager(Storage storage, IDatabaseContextFactory contextFactory, RulesetStore rulesets, IAPIProvider api, [NotNull] AudioManager audioManager, IResourceStore<byte[]> resources, GameHost host, WorkingBeatmap defaultBeatmap, WorkingBeatmap testBeatmap)
+                : base(storage, contextFactory, rulesets, api, audioManager, resources, host, defaultBeatmap, false)
+            {
+                this.testBeatmap = testBeatmap;
+            }
+
+            protected override string ComputeHash(BeatmapSetInfo item, ArchiveReader reader = null)
+                => string.Empty;
+
+            public override WorkingBeatmap GetWorkingBeatmap(BeatmapInfo beatmapInfo)
+                => testBeatmap;
+
+            public override void Save(BeatmapInfo info, IBeatmap beatmapContent, ISkin beatmapSkin = null)
+            {
+                // don't actually care about saving for this context.
+            }
+        }
     }
 }

osu.Game/Tests/Visual/LegacySkinPlayerTestScene.cs

@@ -3,6 +3,8 @@
 
 using NUnit.Framework;
 using osu.Framework.Allocation;
+using osu.Framework.Extensions.IEnumerableExtensions;
+using osu.Framework.Testing;
 using osu.Game.Rulesets;
 using osu.Game.Skinning;
 
@@ -11,6 +13,8 @@ namespace osu.Game.Tests.Visual
     [TestFixture]
     public abstract class LegacySkinPlayerTestScene : PlayerTestScene
     {
+        protected LegacySkin LegacySkin { get; private set; }
+
         private ISkinSource legacySkinSource;
 
         protected override TestPlayer CreatePlayer(Ruleset ruleset) => new SkinProvidingPlayer(legacySkinSource);
@@ -18,8 +22,31 @@ namespace osu.Game.Tests.Visual
         [BackgroundDependencyLoader]
         private void load(SkinManager skins)
         {
-            var legacySkin = new DefaultLegacySkin(skins);
+            LegacySkin = new DefaultLegacySkin(skins);
-            legacySkinSource = new SkinProvidingContainer(legacySkin);
+            legacySkinSource = new SkinProvidingContainer(LegacySkin);
+        }
+
+        [SetUpSteps]
+        public override void SetUpSteps()
+        {
+            base.SetUpSteps();
+            addResetTargetsStep();
+        }
+
+        [TearDownSteps]
+        public override void TearDownSteps()
+        {
+            addResetTargetsStep();
+            base.TearDownSteps();
+        }
+
+        private void addResetTargetsStep()
+        {
+            AddStep("reset targets", () => this.ChildrenOfType<SkinnableTargetContainer>().ForEach(t =>
+            {
+                LegacySkin.ResetDrawableTarget(t);
+                t.Reload();
+            }));
         }
 
         public class SkinProvidingPlayer : TestPlayer