From 351f217c8c54f96df61aebafb1ae7273087cc89c Mon Sep 17 00:00:00 2001 From: Cootz Date: Thu, 29 Jun 2023 08:07:43 +0300 Subject: [PATCH 01/11] Reassign existing scores to new/re-exported beatmap --- osu.Game/Beatmaps/BeatmapImporter.cs | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/osu.Game/Beatmaps/BeatmapImporter.cs b/osu.Game/Beatmaps/BeatmapImporter.cs index 7d367ef77d..04c00ed98a 100644 --- a/osu.Game/Beatmaps/BeatmapImporter.cs +++ b/osu.Game/Beatmaps/BeatmapImporter.cs @@ -20,6 +20,7 @@ using osu.Game.IO; using osu.Game.IO.Archives; using osu.Game.Overlays.Notifications; using osu.Game.Rulesets; +using osu.Game.Scoring; using Realms; namespace osu.Game.Beatmaps @@ -199,6 +200,16 @@ namespace osu.Game.Beatmaps LogForModel(beatmapSet, $"Found existing beatmap set with same OnlineID ({beatmapSet.OnlineID}). It will be disassociated and marked for deletion."); } } + + //Because of specific score storing in Osu! database can already contain scores for imported beatmap. + //To restore scores we need to manually reassign them to new/re-exported beatmap. + foreach (BeatmapInfo beatmap in beatmapSet.Beatmaps) + { + IQueryable scores = realm.All().Where(score => score.BeatmapHash == beatmap.Hash); + + if (scores.Any()) + scores.ForEach(score => score.BeatmapInfo = beatmap); //We intentionally ignore BeatmapHash because we checked hash equality + } } protected override void PostImport(BeatmapSetInfo model, Realm realm, ImportParameters parameters) From 47ccbddfb1d0d3a583a649dd531a6dfd63120ed2 Mon Sep 17 00:00:00 2001 From: Cootz Date: Thu, 29 Jun 2023 08:08:10 +0300 Subject: [PATCH 02/11] Reword comment --- osu.Game/Beatmaps/BeatmapImporter.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Beatmaps/BeatmapImporter.cs b/osu.Game/Beatmaps/BeatmapImporter.cs index 04c00ed98a..bf549f40c4 100644 --- a/osu.Game/Beatmaps/BeatmapImporter.cs +++ b/osu.Game/Beatmaps/BeatmapImporter.cs @@ -201,7 +201,7 @@ namespace osu.Game.Beatmaps } } - //Because of specific score storing in Osu! database can already contain scores for imported beatmap. + //Because of specific score storing in Osu! database, it can already contain scores for imported beatmap. //To restore scores we need to manually reassign them to new/re-exported beatmap. foreach (BeatmapInfo beatmap in beatmapSet.Beatmaps) { From 428383708c1ec2483ba2c39b8b6575aa28e09537 Mon Sep 17 00:00:00 2001 From: Cootz Date: Fri, 30 Jun 2023 20:13:14 +0300 Subject: [PATCH 03/11] Add test for import --- .../Database/BeatmapImporterTests.cs | 68 +++++++++++++++---- 1 file changed, 56 insertions(+), 12 deletions(-) diff --git a/osu.Game.Tests/Database/BeatmapImporterTests.cs b/osu.Game.Tests/Database/BeatmapImporterTests.cs index 446eb72b04..53e5fec075 100644 --- a/osu.Game.Tests/Database/BeatmapImporterTests.cs +++ b/osu.Game.Tests/Database/BeatmapImporterTests.cs @@ -18,6 +18,7 @@ using osu.Game.Extensions; using osu.Game.Models; using osu.Game.Overlays.Notifications; using osu.Game.Rulesets; +using osu.Game.Scoring; using osu.Game.Tests.Resources; using Realms; using SharpCompress.Archives; @@ -416,6 +417,51 @@ namespace osu.Game.Tests.Database }); } + [Test] + public void TestImport_ThenChangeMapWithScore_ThenImport() + { + RunTestWithRealmAsync(async (realm, storage) => + { + var importer = new BeatmapImporter(storage, realm); + + string? temp = TestResources.GetTestBeatmapForImport(); + + var imported = await LoadOszIntoStore(importer, realm.Realm); + + await createScoreForBeatmap(realm.Realm, imported.Beatmaps.First()); + + //editor work imitation + realm.Run(r => + { + r.Write(() => + { + BeatmapInfo beatmap = imported.Beatmaps.First(); + beatmap.Hash = "new_hash"; + beatmap.ResetOnlineInfo(); + }); + }); + + Assert.That(imported.Beatmaps.First().Scores.Any()); + + var importedSecondTime = await importer.Import(new ImportTask(temp)); + + EnsureLoaded(realm.Realm); + + // check the newly "imported" beatmap is not the original. + Assert.NotNull(importedSecondTime); + Debug.Assert(importedSecondTime != null); + Assert.That(imported.ID != importedSecondTime.ID); + + var importedFirstTimeBeatmap = imported.Beatmaps.First(); + var importedSecondTimeBeatmap = importedSecondTime.PerformRead(s => s.Beatmaps.First()); + + Assert.That(importedFirstTimeBeatmap.ID != importedSecondTimeBeatmap.ID); + Assert.That(importedFirstTimeBeatmap.Hash != importedSecondTimeBeatmap.Hash); + Assert.That(!importedFirstTimeBeatmap.Scores.Any()); + Assert.That(importedSecondTimeBeatmap.Scores.Count() == 1); + }); + } + [Test] public void TestImportThenImportWithChangedFile() { @@ -1074,18 +1120,16 @@ namespace osu.Game.Tests.Database Assert.IsTrue(realm.All().First(_ => true).DeletePending); } - private static Task createScoreForBeatmap(Realm realm, BeatmapInfo beatmap) - { - // TODO: reimplement when we have score support in realm. - // return ImportScoreTest.LoadScoreIntoOsu(osu, new ScoreInfo - // { - // OnlineID = 2, - // Beatmap = beatmap, - // BeatmapInfoID = beatmap.ID - // }, new ImportScoreTest.TestArchiveReader()); - - return Task.CompletedTask; - } + private static Task createScoreForBeatmap(Realm realm, BeatmapInfo beatmap) => + realm.WriteAsync(() => + { + realm.Add(new ScoreInfo + { + OnlineID = 2, + BeatmapInfo = beatmap, + BeatmapHash = beatmap.Hash + }); + }); private static void checkBeatmapSetCount(Realm realm, int expected, bool includeDeletePending = false) { From f5d3a2458272fe6de9b8024e56d250318df1b0d2 Mon Sep 17 00:00:00 2001 From: Cootz Date: Fri, 30 Jun 2023 20:15:38 +0300 Subject: [PATCH 04/11] Rename test --- osu.Game.Tests/Database/BeatmapImporterTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Database/BeatmapImporterTests.cs b/osu.Game.Tests/Database/BeatmapImporterTests.cs index 53e5fec075..1f42979d11 100644 --- a/osu.Game.Tests/Database/BeatmapImporterTests.cs +++ b/osu.Game.Tests/Database/BeatmapImporterTests.cs @@ -418,7 +418,7 @@ namespace osu.Game.Tests.Database } [Test] - public void TestImport_ThenChangeMapWithScore_ThenImport() + public void TestImport_ThenModifyMapWithScore_ThenImport() { RunTestWithRealmAsync(async (realm, storage) => { From 2b1d637292621f9d5edfd73b142b30bb5485a6c7 Mon Sep 17 00:00:00 2001 From: Cootz Date: Sat, 1 Jul 2023 09:48:42 +0300 Subject: [PATCH 05/11] Add missing ruleset store --- osu.Game.Tests/Database/BeatmapImporterTests.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/osu.Game.Tests/Database/BeatmapImporterTests.cs b/osu.Game.Tests/Database/BeatmapImporterTests.cs index 1f42979d11..69496630ce 100644 --- a/osu.Game.Tests/Database/BeatmapImporterTests.cs +++ b/osu.Game.Tests/Database/BeatmapImporterTests.cs @@ -423,6 +423,7 @@ namespace osu.Game.Tests.Database RunTestWithRealmAsync(async (realm, storage) => { var importer = new BeatmapImporter(storage, realm); + using var store = new RealmRulesetStore(realm, storage); string? temp = TestResources.GetTestBeatmapForImport(); From 8d25e2c3e1d419a18cf40ff99f2d9cd454991d91 Mon Sep 17 00:00:00 2001 From: Cootz Date: Sat, 1 Jul 2023 09:49:06 +0300 Subject: [PATCH 06/11] Add importer update test --- .../Database/BeatmapImporterUpdateTests.cs | 65 +++++++++++++++++++ osu.Game/Scoring/ScoreInfo.cs | 1 + 2 files changed, 66 insertions(+) diff --git a/osu.Game.Tests/Database/BeatmapImporterUpdateTests.cs b/osu.Game.Tests/Database/BeatmapImporterUpdateTests.cs index b94cff2a9a..20fe0bfca6 100644 --- a/osu.Game.Tests/Database/BeatmapImporterUpdateTests.cs +++ b/osu.Game.Tests/Database/BeatmapImporterUpdateTests.cs @@ -347,6 +347,71 @@ namespace osu.Game.Tests.Database }); } + [Test] + public void TestDandlingScoreTransferred() + { + RunTestWithRealmAsync(async (realm, storage) => + { + var importer = new BeatmapImporter(storage, realm); + using var rulesets = new RealmRulesetStore(realm, storage); + + using var __ = getBeatmapArchive(out string pathOriginal); + using var _ = getBeatmapArchive(out string pathOnlineCopy); + + var importBeforeUpdate = await importer.Import(new ImportTask(pathOriginal)); + + Assert.That(importBeforeUpdate, Is.Not.Null); + Debug.Assert(importBeforeUpdate != null); + + string scoreTargetBeatmapHash = string.Empty; + + //Set score + importBeforeUpdate.PerformWrite(s => + { + var beatmapInfo = s.Beatmaps.First(); + + scoreTargetBeatmapHash = beatmapInfo.Hash; + + s.Realm.Add(new ScoreInfo(beatmapInfo, s.Realm.All().First(), new RealmUser())); + }); + + //Modify beatmap + const string new_beatmap_hash = "new_hash"; + importBeforeUpdate.PerformWrite(s => + { + var beatmapInfo = s.Beatmaps.First(b => b.Hash == scoreTargetBeatmapHash); + + beatmapInfo.Hash = new_beatmap_hash; + beatmapInfo.ResetOnlineInfo(); + }); + + realm.Run(r => r.Refresh()); + + checkCount(realm, 1); + + //second import matches first before modification, + //in other words beatmap version where score have been set + var importAfterUpdate = await importer.ImportAsUpdate(new ProgressNotification(), new ImportTask(pathOnlineCopy), importBeforeUpdate.Value); + + Assert.That(importAfterUpdate, Is.Not.Null); + Debug.Assert(importAfterUpdate != null); + + realm.Run(r => r.Refresh()); + + //account modified beatmap + checkCount(realm, count_beatmaps + 1); + checkCount(realm, count_beatmaps + 1); + checkCount(realm, 2); + + // score is transferred across to the new set + checkCount(realm, 1); + + //score is transferred to new beatmap + Assert.That(importBeforeUpdate.Value.Beatmaps.First(b => b.Hash == new_beatmap_hash).Scores, Has.Count.EqualTo(0)); + Assert.That(importAfterUpdate.Value.Beatmaps.First(b => b.Hash == scoreTargetBeatmapHash).Scores, Has.Count.EqualTo(1)); + }); + } + [Test] public void TestScoreLostOnModification() { diff --git a/osu.Game/Scoring/ScoreInfo.cs b/osu.Game/Scoring/ScoreInfo.cs index d56338c6a4..6dfac419e5 100644 --- a/osu.Game/Scoring/ScoreInfo.cs +++ b/osu.Game/Scoring/ScoreInfo.cs @@ -82,6 +82,7 @@ namespace osu.Game.Scoring { Ruleset = ruleset ?? new RulesetInfo(); BeatmapInfo = beatmap ?? new BeatmapInfo(); + BeatmapHash = BeatmapInfo.Hash; RealmUser = realmUser ?? new RealmUser(); ID = Guid.NewGuid(); } From a4a9223726b35d5dcba09af3fde12c58d0b3063a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sat, 1 Jul 2023 23:12:04 +0900 Subject: [PATCH 07/11] Move score re-attach to `PostImport` --- osu.Game/Beatmaps/BeatmapImporter.cs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapImporter.cs b/osu.Game/Beatmaps/BeatmapImporter.cs index bf549f40c4..b0f58d0298 100644 --- a/osu.Game/Beatmaps/BeatmapImporter.cs +++ b/osu.Game/Beatmaps/BeatmapImporter.cs @@ -200,21 +200,22 @@ namespace osu.Game.Beatmaps LogForModel(beatmapSet, $"Found existing beatmap set with same OnlineID ({beatmapSet.OnlineID}). It will be disassociated and marked for deletion."); } } + } + + protected override void PostImport(BeatmapSetInfo model, Realm realm, ImportParameters parameters) + { + base.PostImport(model, realm, parameters); //Because of specific score storing in Osu! database, it can already contain scores for imported beatmap. //To restore scores we need to manually reassign them to new/re-exported beatmap. - foreach (BeatmapInfo beatmap in beatmapSet.Beatmaps) + foreach (BeatmapInfo beatmap in model.Beatmaps) { IQueryable scores = realm.All().Where(score => score.BeatmapHash == beatmap.Hash); if (scores.Any()) scores.ForEach(score => score.BeatmapInfo = beatmap); //We intentionally ignore BeatmapHash because we checked hash equality } - } - protected override void PostImport(BeatmapSetInfo model, Realm realm, ImportParameters parameters) - { - base.PostImport(model, realm, parameters); ProcessBeatmap?.Invoke(model, parameters.Batch ? MetadataLookupScope.LocalCacheFirst : MetadataLookupScope.OnlineFirst); } From 5bd91a531d37c4a89e7cc1925ca80888c81d276b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Sat, 1 Jul 2023 23:37:22 +0900 Subject: [PATCH 08/11] Tidy up comments and code --- osu.Game/Beatmaps/BeatmapImporter.cs | 10 ++++------ 1 file changed, 4 insertions(+), 6 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapImporter.cs b/osu.Game/Beatmaps/BeatmapImporter.cs index b0f58d0298..da987eb752 100644 --- a/osu.Game/Beatmaps/BeatmapImporter.cs +++ b/osu.Game/Beatmaps/BeatmapImporter.cs @@ -206,14 +206,12 @@ namespace osu.Game.Beatmaps { base.PostImport(model, realm, parameters); - //Because of specific score storing in Osu! database, it can already contain scores for imported beatmap. - //To restore scores we need to manually reassign them to new/re-exported beatmap. + // Scores are stored separately from beatmaps, and persisted when a beatmap is modified or deleted. + // Let's reattach any matching scores that exist in the database, based on hash. foreach (BeatmapInfo beatmap in model.Beatmaps) { - IQueryable scores = realm.All().Where(score => score.BeatmapHash == beatmap.Hash); - - if (scores.Any()) - scores.ForEach(score => score.BeatmapInfo = beatmap); //We intentionally ignore BeatmapHash because we checked hash equality + foreach (var score in realm.All().Where(score => score.BeatmapHash == beatmap.Hash)) + score.BeatmapInfo = beatmap; } ProcessBeatmap?.Invoke(model, parameters.Batch ? MetadataLookupScope.LocalCacheFirst : MetadataLookupScope.OnlineFirst); From 1ce60378be4ced5619533c2aaa25d46667dca777 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sat, 1 Jul 2023 20:37:33 +0200 Subject: [PATCH 09/11] Rewrite comments further --- osu.Game.Tests/Database/BeatmapImporterTests.cs | 5 ++++- .../Database/BeatmapImporterUpdateTests.cs | 16 +++++++++------- osu.Game/Beatmaps/BeatmapImporter.cs | 2 +- 3 files changed, 14 insertions(+), 9 deletions(-) diff --git a/osu.Game.Tests/Database/BeatmapImporterTests.cs b/osu.Game.Tests/Database/BeatmapImporterTests.cs index 69496630ce..84e84f030c 100644 --- a/osu.Game.Tests/Database/BeatmapImporterTests.cs +++ b/osu.Game.Tests/Database/BeatmapImporterTests.cs @@ -431,7 +431,7 @@ namespace osu.Game.Tests.Database await createScoreForBeatmap(realm.Realm, imported.Beatmaps.First()); - //editor work imitation + // imitate making local changes via editor realm.Run(r => { r.Write(() => @@ -442,6 +442,9 @@ namespace osu.Game.Tests.Database }); }); + // for now, making changes to a beatmap doesn't remove the backlink from the score to the beatmap. + // the logic of ensuring that scores match the beatmap is upheld via comparing the hash in usages (see: https://github.com/ppy/osu/pull/22539). + // TODO: revisit when fixing https://github.com/ppy/osu/issues/24069. Assert.That(imported.Beatmaps.First().Scores.Any()); var importedSecondTime = await importer.Import(new ImportTask(temp)); diff --git a/osu.Game.Tests/Database/BeatmapImporterUpdateTests.cs b/osu.Game.Tests/Database/BeatmapImporterUpdateTests.cs index 20fe0bfca6..8f48a96ac9 100644 --- a/osu.Game.Tests/Database/BeatmapImporterUpdateTests.cs +++ b/osu.Game.Tests/Database/BeatmapImporterUpdateTests.cs @@ -365,7 +365,7 @@ namespace osu.Game.Tests.Database string scoreTargetBeatmapHash = string.Empty; - //Set score + // set a score on the beatmap importBeforeUpdate.PerformWrite(s => { var beatmapInfo = s.Beatmaps.First(); @@ -375,7 +375,7 @@ namespace osu.Game.Tests.Database s.Realm.Add(new ScoreInfo(beatmapInfo, s.Realm.All().First(), new RealmUser())); }); - //Modify beatmap + // locally modify beatmap const string new_beatmap_hash = "new_hash"; importBeforeUpdate.PerformWrite(s => { @@ -387,10 +387,12 @@ namespace osu.Game.Tests.Database realm.Run(r => r.Refresh()); + // for now, making changes to a beatmap doesn't remove the backlink from the score to the beatmap. + // the logic of ensuring that scores match the beatmap is upheld via comparing the hash in usages (https://github.com/ppy/osu/pull/22539). + // TODO: revisit when fixing https://github.com/ppy/osu/issues/24069. checkCount(realm, 1); - //second import matches first before modification, - //in other words beatmap version where score have been set + // reimport the original beatmap before local modifications var importAfterUpdate = await importer.ImportAsUpdate(new ProgressNotification(), new ImportTask(pathOnlineCopy), importBeforeUpdate.Value); Assert.That(importAfterUpdate, Is.Not.Null); @@ -398,15 +400,15 @@ namespace osu.Game.Tests.Database realm.Run(r => r.Refresh()); - //account modified beatmap + // both original and locally modified versions present checkCount(realm, count_beatmaps + 1); checkCount(realm, count_beatmaps + 1); checkCount(realm, 2); - // score is transferred across to the new set + // score is preserved checkCount(realm, 1); - //score is transferred to new beatmap + // score is transferred to new beatmap Assert.That(importBeforeUpdate.Value.Beatmaps.First(b => b.Hash == new_beatmap_hash).Scores, Has.Count.EqualTo(0)); Assert.That(importAfterUpdate.Value.Beatmaps.First(b => b.Hash == scoreTargetBeatmapHash).Scores, Has.Count.EqualTo(1)); }); diff --git a/osu.Game/Beatmaps/BeatmapImporter.cs b/osu.Game/Beatmaps/BeatmapImporter.cs index da987eb752..fd766490fc 100644 --- a/osu.Game/Beatmaps/BeatmapImporter.cs +++ b/osu.Game/Beatmaps/BeatmapImporter.cs @@ -206,7 +206,7 @@ namespace osu.Game.Beatmaps { base.PostImport(model, realm, parameters); - // Scores are stored separately from beatmaps, and persisted when a beatmap is modified or deleted. + // Scores are stored separately from beatmaps, and persist even when a beatmap is modified or deleted. // Let's reattach any matching scores that exist in the database, based on hash. foreach (BeatmapInfo beatmap in model.Beatmaps) { From b6e9422aa4c714799f5f5fcf26010a8a60baedba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sat, 1 Jul 2023 20:38:00 +0200 Subject: [PATCH 10/11] Fix typo in test name --- osu.Game.Tests/Database/BeatmapImporterUpdateTests.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Database/BeatmapImporterUpdateTests.cs b/osu.Game.Tests/Database/BeatmapImporterUpdateTests.cs index 8f48a96ac9..83cb54df3f 100644 --- a/osu.Game.Tests/Database/BeatmapImporterUpdateTests.cs +++ b/osu.Game.Tests/Database/BeatmapImporterUpdateTests.cs @@ -348,7 +348,7 @@ namespace osu.Game.Tests.Database } [Test] - public void TestDandlingScoreTransferred() + public void TestDanglingScoreTransferred() { RunTestWithRealmAsync(async (realm, storage) => { From 746212be7b4c1d8b44517a1d4aeecd5709ff64f0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Sat, 1 Jul 2023 20:42:34 +0200 Subject: [PATCH 11/11] Remove weird `.Run(r => r.Write(...))` construction --- osu.Game.Tests/Database/BeatmapImporterTests.cs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/osu.Game.Tests/Database/BeatmapImporterTests.cs b/osu.Game.Tests/Database/BeatmapImporterTests.cs index 84e84f030c..84e6a6c00f 100644 --- a/osu.Game.Tests/Database/BeatmapImporterTests.cs +++ b/osu.Game.Tests/Database/BeatmapImporterTests.cs @@ -432,14 +432,12 @@ namespace osu.Game.Tests.Database await createScoreForBeatmap(realm.Realm, imported.Beatmaps.First()); // imitate making local changes via editor - realm.Run(r => + // ReSharper disable once MethodHasAsyncOverload + realm.Write(_ => { - r.Write(() => - { - BeatmapInfo beatmap = imported.Beatmaps.First(); - beatmap.Hash = "new_hash"; - beatmap.ResetOnlineInfo(); - }); + BeatmapInfo beatmap = imported.Beatmaps.First(); + beatmap.Hash = "new_hash"; + beatmap.ResetOnlineInfo(); }); // for now, making changes to a beatmap doesn't remove the backlink from the score to the beatmap.