Merge pull request #19025 from peppy/custom-ruleset-score-import

Fix custom rulesets not importing scores at all

osu.Game.Tests/Visual/Gameplay/TestScenePlayerLocalScoreImport.cs

@@ -0,0 +1,107 @@
+// 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.Allocation;
+using osu.Framework.Audio;
+using osu.Framework.Extensions;
+using osu.Framework.Extensions.ObjectExtensions;
+using osu.Framework.Platform;
+using osu.Framework.Screens;
+using osu.Game.Beatmaps;
+using osu.Game.Rulesets;
+using osu.Game.Rulesets.Objects;
+using osu.Game.Rulesets.Osu;
+using osu.Game.Rulesets.Scoring;
+using osu.Game.Scoring;
+using osu.Game.Screens.Ranking;
+using osu.Game.Tests.Resources;
+
+namespace osu.Game.Tests.Visual.Gameplay
+{
+    public class TestScenePlayerLocalScoreImport : PlayerTestScene
+    {
+        private BeatmapManager beatmaps = null!;
+        private RulesetStore rulesets = null!;
+
+        private BeatmapSetInfo? importedSet;
+
+        [BackgroundDependencyLoader]
+        private void load(GameHost host, AudioManager audio)
+        {
+            Dependencies.Cache(rulesets = new RealmRulesetStore(Realm));
+            Dependencies.Cache(beatmaps = new BeatmapManager(LocalStorage, Realm, rulesets, null, audio, Resources, host, Beatmap.Default));
+            Dependencies.Cache(new ScoreManager(rulesets, () => beatmaps, LocalStorage, Realm, Scheduler));
+            Dependencies.Cache(Realm);
+        }
+
+        public override void SetUpSteps()
+        {
+            base.SetUpSteps();
+
+            AddStep("import beatmap", () =>
+            {
+                beatmaps.Import(TestResources.GetQuickTestBeatmapForImport()).WaitSafely();
+                importedSet = beatmaps.GetAllUsableBeatmapSets().First();
+            });
+        }
+
+        protected override IBeatmap CreateBeatmap(RulesetInfo ruleset) => beatmaps.GetWorkingBeatmap(importedSet?.Beatmaps.First()).Beatmap;
+
+        private Ruleset? customRuleset;
+
+        protected override Ruleset CreatePlayerRuleset() => customRuleset ?? new OsuRuleset();
+
+        protected override TestPlayer CreatePlayer(Ruleset ruleset) => new TestPlayer(false);
+
+        protected override bool HasCustomSteps => true;
+
+        protected override bool AllowFail => false;
+
+        [Test]
+        public void TestScoreStoredLocally()
+        {
+            AddStep("set no custom ruleset", () => customRuleset = null);
+
+            CreateTest();
+
+            AddUntilStep("wait for track to start running", () => Beatmap.Value.Track.IsRunning);
+
+            AddStep("seek to completion", () => Player.GameplayClockContainer.Seek(Player.DrawableRuleset.Objects.Last().GetEndTime()));
+
+            AddUntilStep("results displayed", () => Player.GetChildScreen() is ResultsScreen);
+            AddUntilStep("score in database", () => Realm.Run(r => r.Find<ScoreInfo>(Player.Score.ScoreInfo.ID) != null));
+        }
+
+        [Test]
+        public void TestScoreStoredLocallyCustomRuleset()
+        {
+            Ruleset createCustomRuleset() => new CustomRuleset();
+
+            AddStep("import custom ruleset", () => Realm.Write(r => r.Add(createCustomRuleset().RulesetInfo)));
+            AddStep("set custom ruleset", () => customRuleset = createCustomRuleset());
+
+            CreateTest();
+
+            AddAssert("score has custom ruleset", () => Player.Score.ScoreInfo.Ruleset.Equals(customRuleset.AsNonNull().RulesetInfo));
+
+            AddUntilStep("wait for track to start running", () => Beatmap.Value.Track.IsRunning);
+
+            AddStep("seek to completion", () => Player.GameplayClockContainer.Seek(Player.DrawableRuleset.Objects.Last().GetEndTime()));
+
+            AddUntilStep("results displayed", () => Player.GetChildScreen() is ResultsScreen);
+            AddUntilStep("score in database", () => Realm.Run(r => r.Find<ScoreInfo>(Player.Score.ScoreInfo.ID) != null));
+        }
+
+        private class CustomRuleset : OsuRuleset, ILegacyRuleset
+        {
+            public override string Description => "custom";
+            public override string ShortName => "custom";
+
+            int ILegacyRuleset.LegacyID => -1;
+
+            public override ScoreProcessor CreateScoreProcessor() => new ScoreProcessor(this);
+        }
+    }
+}

osu.Game.Tests/Visual/Gameplay/TestScenePlayerScoreSubmission.cs

@@ -365,21 +365,9 @@ namespace osu.Game.Tests.Visual.Gameplay
 
                 ImportedScore = score;
 
-                // It was discovered that Score members could sometimes be half-populated.
-                // In particular, the RulesetID property could be set to 0 even on non-osu! maps.
-                // We want to test that the state of that property is consistent in this test.
-                // EF makes this impossible.
-                //
-                // First off, because of the EF navigational property-explicit foreign key field duality,
-                // it can happen that - for example - the Ruleset navigational property is correctly initialised to mania,
-                // but the RulesetID foreign key property is not initialised and remains 0.
-                // EF silently bypasses this by prioritising the Ruleset navigational property over the RulesetID foreign key one.
-                //
-                // Additionally, adding an entity to an EF DbSet CAUSES SIDE EFFECTS with regard to the foreign key property.
-                // In the above instance, if a ScoreInfo with Ruleset = {mania} and RulesetID = 0 is attached to an EF context,
-                // RulesetID WILL BE SILENTLY SET TO THE CORRECT VALUE of 3.
-                //
-                // For the above reasons, actual importing is disabled in this test.
+                // Calling base.ImportScore is omitted as it will fail for the test method which uses a custom ruleset.
+                // This can be resolved by doing something similar to what TestScenePlayerLocalScoreImport is doing,
+                // but requires a bit of restructuring.
             }
         }
     }

osu.Game.Tests/Visual/Gameplay/TestSceneReplayDownloadButton.cs

@@ -142,6 +142,28 @@ namespace osu.Game.Tests.Visual.Gameplay
             AddAssert("button is not enabled", () => !downloadButton.ChildrenOfType<DownloadButton>().First().Enabled.Value);
         }
 
+        [Test]
+        public void TestLocallyAvailableWithoutReplay()
+        {
+            Live<ScoreInfo> imported = null;
+
+            AddStep("import score", () => imported = scoreManager.Import(getScoreInfo(false, false)));
+
+            AddStep("create button without replay", () =>
+            {
+                Child = downloadButton = new TestReplayDownloadButton(imported.Value)
+                {
+                    Anchor = Anchor.Centre,
+                    Origin = Anchor.Centre,
+                };
+            });
+
+            AddUntilStep("wait for load", () => downloadButton.IsLoaded);
+
+            AddUntilStep("state is not downloaded", () => downloadButton.State.Value == DownloadState.NotDownloaded);
+            AddAssert("button is not enabled", () => !downloadButton.ChildrenOfType<DownloadButton>().First().Enabled.Value);
+        }
+
         [Test]
         public void TestScoreImportThenDelete()
         {
@@ -189,11 +211,11 @@ namespace osu.Game.Tests.Visual.Gameplay
             AddAssert("button is not enabled", () => !downloadButton.ChildrenOfType<DownloadButton>().First().Enabled.Value);
         }
 
-        private ScoreInfo getScoreInfo(bool replayAvailable)
+        private ScoreInfo getScoreInfo(bool replayAvailable, bool hasOnlineId = true)
         {
             return new APIScore
             {
-                OnlineID = online_score_id,
+                OnlineID = hasOnlineId ? online_score_id : 0,
                 RulesetID = 0,
                 Beatmap = CreateAPIBeatmapSet(new OsuRuleset().RulesetInfo).Beatmaps.First(),
                 HasReplay = replayAvailable,

osu.Game/Screens/Play/Player.cs

@@ -22,6 +22,7 @@ using osu.Framework.Threading;
 using osu.Game.Audio;
 using osu.Game.Beatmaps;
 using osu.Game.Configuration;
+using osu.Game.Extensions;
 using osu.Game.Graphics.Containers;
 using osu.Game.IO.Archives;
 using osu.Game.Online.API;
@@ -1064,12 +1065,15 @@ namespace osu.Game.Screens.Play
             if (DrawableRuleset.ReplayScore != null)
                 return Task.CompletedTask;
 
-            LegacyByteArrayReader replayReader;
+            LegacyByteArrayReader replayReader = null;
 
-            using (var stream = new MemoryStream())
+            if (score.ScoreInfo.Ruleset.IsLegacyRuleset())
             {
-                new LegacyScoreEncoder(score, GameplayState.Beatmap).Encode(stream);
-                replayReader = new LegacyByteArrayReader(stream.ToArray(), "replay.osr");
+                using (var stream = new MemoryStream())
+                {
+                    new LegacyScoreEncoder(score, GameplayState.Beatmap).Encode(stream);
+                    replayReader = new LegacyByteArrayReader(stream.ToArray(), "replay.osr");
+                }
             }
 
             // the import process will re-attach managed beatmap/rulesets to this score. we don't want this for now, so create a temporary copy to import.

osu.Game/Tests/Visual/PlayerTestScene.cs

@@ -49,7 +49,7 @@ namespace osu.Game.Tests.Visual
 
             action?.Invoke();
 
-            AddStep(CreatePlayerRuleset().Description, LoadPlayer);
+            AddStep($"Load player for {CreatePlayerRuleset().Description}", LoadPlayer);
             AddUntilStep("player loaded", () => Player.IsLoaded && Player.Alpha == 1);
         }