From 56ef5eae1409622518fbc19872d5e3477abe90a2 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 26 Dec 2025 22:36:16 +0900 Subject: [PATCH] Revert "Merge pull request #36102 from bdach/move-check-to-better-place" This reverts commit 2cb2167765a2c5514841657a63845d2c4d078649, reversing changes made to 0bcb3c5839ac300c689e4edf6582a94dbf8cb748. --- .../Database/BeatmapImporterTests.cs | 14 +--- osu.Game/Beatmaps/BeatmapImporter.cs | 72 ++++++++++--------- 2 files changed, 40 insertions(+), 46 deletions(-) diff --git a/osu.Game.Tests/Database/BeatmapImporterTests.cs b/osu.Game.Tests/Database/BeatmapImporterTests.cs index bdba1c368f..f3ca665380 100644 --- a/osu.Game.Tests/Database/BeatmapImporterTests.cs +++ b/osu.Game.Tests/Database/BeatmapImporterTests.cs @@ -778,19 +778,7 @@ namespace osu.Game.Tests.Database { RunTestWithRealmAsync(async (realm, storage) => { - var importer = new BeatmapImporter(storage, realm) - { - ProcessBeatmap = (set, _) => - { - set.OnlineID = 1234; - // note: this is only intended to be a quick-and-easy approximation of proper operation - // the more proper way to do this would be to assign IDs based on content hash or something, but it largely doesn't matter here - // so long as the beatmaps in the set get assigned an ID by the process flow - for (int i = 0; i < set.Beatmaps.Count; ++i) - set.Beatmaps[i].OnlineID = 5678 + i; - } - }; - + var importer = new BeatmapImporter(storage, realm); using var store = new RealmRulesetStore(realm, storage); string? pathOriginal = TestResources.GetTestBeatmapForImport(); diff --git a/osu.Game/Beatmaps/BeatmapImporter.cs b/osu.Game/Beatmaps/BeatmapImporter.cs index d8508ce6de..f80c4de4ea 100644 --- a/osu.Game/Beatmaps/BeatmapImporter.cs +++ b/osu.Game/Beatmaps/BeatmapImporter.cs @@ -182,6 +182,45 @@ namespace osu.Game.Beatmaps } validateOnlineIds(beatmapSet, realm); + + bool hadOnlineIDs = beatmapSet.Beatmaps.Any(b => b.OnlineID > 0); + + // TODO: this may no longer be valid as we aren't doing an online population at this point. + // ensure at least one beatmap was able to retrieve or keep an online ID, else drop the set ID. + if (hadOnlineIDs && !beatmapSet.Beatmaps.Any(b => b.OnlineID > 0)) + { + if (beatmapSet.OnlineID > 0) + { + beatmapSet.OnlineID = -1; + LogForModel(beatmapSet, "Disassociating beatmap set ID due to loss of all beatmap IDs"); + } + } + } + + protected override void PreImport(BeatmapSetInfo beatmapSet, Realm realm) + { + // We are about to import a new beatmap. Before doing so, ensure that no other set shares the online IDs used by the new one. + // Note that this means if the previous beatmap is restored by the user, it will no longer be linked to its online IDs. + // If this is ever an issue, we can consider marking as pending delete but not resetting the IDs (but care will be required for + // beatmaps, which don't have their own `DeletePending` state). + + if (beatmapSet.OnlineID > 0) + { + // Required local for iOS. Will cause runtime crash if inlined. + int onlineId = beatmapSet.OnlineID; + + // OnlineID should really be unique, but to avoid catastrophic failure let's iterate just to be sure. + foreach (var existingSetWithSameOnlineID in realm.All().Where(b => b.OnlineID == onlineId)) + { + existingSetWithSameOnlineID.DeletePending = true; + existingSetWithSameOnlineID.OnlineID = -1; + + foreach (var b in existingSetWithSameOnlineID.Beatmaps) + b.ResetOnlineInfo(); + + 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) @@ -196,39 +235,6 @@ namespace osu.Game.Beatmaps } ProcessBeatmap?.Invoke(model, parameters.Batch ? MetadataLookupScope.LocalCacheFirst : MetadataLookupScope.OnlineFirst); - - bool hadOnlineIDs = model.OnlineID > 0 || model.Beatmaps.Any(b => b.OnlineID > 0); - - // ensure at least one beatmap was able to retrieve or keep an online ID, else drop the set ID. - if (hadOnlineIDs && !model.Beatmaps.Any(b => b.OnlineID > 0)) - { - model.OnlineID = -1; - LogForModel(model, "Disassociating beatmap set ID due to loss of all beatmap IDs"); - } - - // Before concluding the import, ensure that no other set shares the online IDs used by the new one. - // Note that this means if the previous beatmap is restored by the user, it will no longer be linked to its online IDs. - // If this is ever an issue, we can consider marking as pending delete but not resetting the IDs (but care will be required for - // beatmaps, which don't have their own `DeletePending` state). - - if (model.OnlineID > 0) - { - // Required local for iOS. Will cause runtime crash if inlined. - int onlineId = model.OnlineID; - Guid guid = model.ID; - - // OnlineID should really be unique, but to avoid catastrophic failure let's iterate just to be sure. - foreach (var existingSetWithSameOnlineID in realm.All().Where(b => b.OnlineID == onlineId && b.ID != guid)) - { - existingSetWithSameOnlineID.DeletePending = true; - existingSetWithSameOnlineID.OnlineID = -1; - - foreach (var b in existingSetWithSameOnlineID.Beatmaps) - b.ResetOnlineInfo(); - - LogForModel(model, $"Found existing beatmap set with same OnlineID ({model.OnlineID}). It will be disassociated and marked for deletion."); - } - } } private void validateOnlineIds(BeatmapSetInfo beatmapSet, Realm realm)