From 3b7018fcd69be54032d0e27fb04c68cde875f461 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 9 Feb 2018 17:22:48 +0900 Subject: [PATCH 01/33] Simplify beatmap import process --- osu.Game/Beatmaps/BeatmapManager.cs | 174 ++++++++++++++-------------- 1 file changed, 87 insertions(+), 87 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index 44289e2400..8b8a8e197a 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -194,50 +194,74 @@ namespace osu.Game.Beatmaps } private readonly object importContextLock = new object(); - private Lazy importContext; /// /// Import a beatmap from an . /// - /// The beatmap to be imported. - public BeatmapSetInfo Import(ArchiveReader archiveReader) + /// The beatmap to be imported. + public BeatmapSetInfo Import(ArchiveReader archive) { - // let's only allow one concurrent import at a time for now. + // let's only allow one concurrent import at a time for now lock (importContextLock) { var context = importContext.Value; using (var transaction = context.BeginTransaction()) { - // create local stores so we can isolate and thread safely, and share a context/transaction. - var iFiles = new FileStore(() => context, storage); - var iBeatmaps = createBeatmapStore(() => context); + // create a new set info (don't yet add to database) + var beatmapSet = createBeatmapSetInfo(archive); - BeatmapSetInfo set = importToStorage(iFiles, iBeatmaps, archiveReader); - - if (set.ID == 0) + // check if this beatmap has already been imported and exit early if so + var existingHashMatch = beatmaps.BeatmapSets.FirstOrDefault(b => b.Hash == beatmapSet.Hash); + if (existingHashMatch != null) { - iBeatmaps.Add(set); - context.SaveChanges(); + undelete(beatmaps, files, existingHashMatch); + return existingHashMatch; } + // check if a set already exists with the same online id + if (beatmapSet.OnlineBeatmapSetID != null) + { + var existingOnlineId = beatmaps.BeatmapSets.FirstOrDefault(b => b.OnlineBeatmapSetID == beatmapSet.OnlineBeatmapSetID); + if (existingOnlineId != null) + Delete(existingOnlineId); + } + + beatmapSet.Files = createFileInfos(archive, new FileStore(() => context, storage)); + beatmapSet.Beatmaps = createBeatmapDifficulties(archive); + + // remove metadata from difficulties where it matches the set + foreach (BeatmapInfo b in beatmapSet.Beatmaps) + if (beatmapSet.Metadata.Equals(b.Metadata)) + b.Metadata = null; + + // import to beatmap store + import(beatmapSet, context); + context.SaveChanges(transaction); - return set; + return beatmapSet; } } } + /// /// Import a beatmap from a . /// /// The beatmap to be imported. public void Import(BeatmapSetInfo beatmapSetInfo) { - // If we have an ID then we already exist in the database. - if (beatmapSetInfo.ID != 0) return; + lock (importContextLock) + { + var context = importContext.Value; - createBeatmapStore(createContext).Add(beatmapSetInfo); + using (var transaction = context.BeginTransaction()) + { + import(beatmapSetInfo, context); + context.SaveChanges(transaction); + } + } } /// @@ -495,6 +519,8 @@ namespace osu.Game.Beatmaps /// Results from the provided query. public IEnumerable QueryBeatmaps(Expression> query) => beatmaps.Beatmaps.AsNoTracking().Where(query); + private void import(BeatmapSetInfo beatmapSet, OsuDbContext context) => createBeatmapStore(() => context).Add(beatmapSet); + /// /// Creates an from a valid storage path. /// @@ -508,49 +534,43 @@ namespace osu.Game.Beatmaps return new LegacyFilesystemReader(path); } - /// - /// Import a beamap into our local storage. - /// If the beatmap is already imported, the existing instance will be returned. - /// - /// The store to import beatmap files to. - /// The store to import beatmaps to. - /// The beatmap archive to be read. - /// The imported beatmap, or an existing instance if it is already present. - private BeatmapSetInfo importToStorage(FileStore files, BeatmapStore beatmaps, ArchiveReader reader) + private string computeBeatmapSetHash(ArchiveReader reader) { - // let's make sure there are actually .osu files to import. - string mapName = reader.Filenames.FirstOrDefault(f => f.EndsWith(".osu")); - if (string.IsNullOrEmpty(mapName)) - throw new InvalidOperationException("No beatmap files found in the map folder."); - // for now, concatenate all .osu files in the set to create a unique hash. MemoryStream hashable = new MemoryStream(); foreach (string file in reader.Filenames.Where(f => f.EndsWith(".osu"))) using (Stream s = reader.GetStream(file)) s.CopyTo(hashable); - var hash = hashable.ComputeSHA2Hash(); + return hashable.ComputeSHA2Hash(); + } - // check if this beatmap has already been imported and exit early if so. - var beatmapSet = beatmaps.BeatmapSets.FirstOrDefault(b => b.Hash == hash); + /// + /// + /// + /// + /// + private BeatmapSetInfo createBeatmapSetInfo(ArchiveReader reader) + { + // let's make sure there are actually .osu files to import. + string mapName = reader.Filenames.FirstOrDefault(f => f.EndsWith(".osu")); + if (string.IsNullOrEmpty(mapName)) throw new InvalidOperationException("No beatmap files found in the map folder."); - if (beatmapSet != null) + BeatmapMetadata metadata; + using (var stream = new StreamReader(reader.GetStream(mapName))) + metadata = Decoder.GetDecoder(stream).DecodeBeatmap(stream).Metadata; + + return new BeatmapSetInfo { - undelete(beatmaps, files, beatmapSet); - - // ensure all files are present and accessible - foreach (var f in beatmapSet.Files) - { - if (!storage.Exists(f.FileInfo.StoragePath)) - using (Stream s = reader.GetStream(f.Filename)) - files.Add(s, false); - } - - // todo: delete any files which shouldn't exist any more. - - return beatmapSet; - } + OnlineBeatmapSetID = metadata.OnlineBeatmapSetID, + Beatmaps = new List(), + Hash = computeBeatmapSetHash(reader), + Metadata = metadata + }; + } + private List createFileInfos(ArchiveReader reader, FileStore files) + { List fileInfos = new List(); // import files to manager @@ -562,28 +582,20 @@ namespace osu.Game.Beatmaps FileInfo = files.Add(s) }); - BeatmapMetadata metadata; + return fileInfos; + } - using (var stream = new StreamReader(reader.GetStream(mapName))) - metadata = Decoder.GetDecoder(stream).DecodeBeatmap(stream).Metadata; + /// + /// Import a beamap into our local storage. + /// If the beatmap is already imported, the existing instance will be returned. + /// + /// The beatmap archive to be read. + /// The imported beatmap, or an existing instance if it is already present. + private List createBeatmapDifficulties(ArchiveReader reader) + { + var beatmapInfos = new List(); - // check if a set already exists with the same online id. - if (metadata.OnlineBeatmapSetID != null) - beatmapSet = beatmaps.BeatmapSets.FirstOrDefault(b => b.OnlineBeatmapSetID == metadata.OnlineBeatmapSetID); - - if (beatmapSet == null) - beatmapSet = new BeatmapSetInfo - { - OnlineBeatmapSetID = metadata.OnlineBeatmapSetID, - Beatmaps = new List(), - Hash = hash, - Files = fileInfos, - Metadata = metadata - }; - - var mapNames = reader.Filenames.Where(f => f.EndsWith(".osu")); - - foreach (var name in mapNames) + foreach (var name in reader.Filenames.Where(f => f.EndsWith(".osu"))) { using (var raw = reader.GetStream(name)) using (var ms = new MemoryStream()) //we need a memory stream so we can seek and shit @@ -599,36 +611,24 @@ namespace osu.Game.Beatmaps beatmap.BeatmapInfo.Hash = ms.ComputeSHA2Hash(); beatmap.BeatmapInfo.MD5Hash = ms.ComputeMD5Hash(); - var existing = beatmaps.Beatmaps.FirstOrDefault(b => b.Hash == beatmap.BeatmapInfo.Hash || beatmap.BeatmapInfo.OnlineBeatmapID != null && b.OnlineBeatmapID == beatmap.BeatmapInfo.OnlineBeatmapID); + RulesetInfo ruleset = rulesets.GetRuleset(beatmap.BeatmapInfo.RulesetID); - if (existing == null) - { - // Exclude beatmap-metadata if it's equal to beatmapset-metadata - if (metadata.Equals(beatmap.Metadata)) - beatmap.BeatmapInfo.Metadata = null; + // TODO: this should be done in a better place once we actually need to dynamically update it. + beatmap.BeatmapInfo.Ruleset = ruleset; + beatmap.BeatmapInfo.StarDifficulty = ruleset?.CreateInstance()?.CreateDifficultyCalculator(beatmap).Calculate() ?? 0; - RulesetInfo ruleset = rulesets.GetRuleset(beatmap.BeatmapInfo.RulesetID); - - // TODO: this should be done in a better place once we actually need to dynamically update it. - beatmap.BeatmapInfo.Ruleset = ruleset; - beatmap.BeatmapInfo.StarDifficulty = ruleset?.CreateInstance()?.CreateDifficultyCalculator(beatmap).Calculate() ?? 0; - - beatmapSet.Beatmaps.Add(beatmap.BeatmapInfo); - } + beatmapInfos.Add(beatmap.BeatmapInfo); } } - return beatmapSet; + return beatmapInfos; } /// /// Returns a list of all usable s. /// /// A list of available . - public List GetAllUsableBeatmapSets() - { - return beatmaps.BeatmapSets.Where(s => !s.DeletePending).ToList(); - } + public List GetAllUsableBeatmapSets() => beatmaps.BeatmapSets.Where(s => !s.DeletePending).ToList(); protected class BeatmapManagerWorkingBeatmap : WorkingBeatmap { From a1669324688c28e5006b36ca8f5a6f72d26a1102 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 9 Feb 2018 17:51:29 +0900 Subject: [PATCH 02/33] Add deletion test --- .../Beatmaps/IO/ImportBeatmapTest.cs | 29 +++++++++++++++++++ osu.Game/Beatmaps/BeatmapManager.cs | 9 ++++-- 2 files changed, 35 insertions(+), 3 deletions(-) diff --git a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs index 0b49bc8bb9..a7ff308c6b 100644 --- a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs +++ b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs @@ -42,6 +42,35 @@ namespace osu.Game.Tests.Beatmaps.IO } } + [Test] + public void TestImportThenDelete() + { + //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. + using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportThenDelete")) + { + var osu = loadOsu(host); + + var temp = prepareTempCopy(osz_path); + Assert.IsTrue(File.Exists(temp)); + + var manager = osu.Dependencies.Get(); + + var imported = manager.Import(temp); + + ensureLoaded(osu); + + manager.Delete(imported.First()); + + Assert.IsTrue(manager.GetAllUsableBeatmapSets().Count == 0); + Assert.IsTrue(manager.QueryBeatmapSets(_ => true).ToList().Count() == 1); + Assert.IsTrue(manager.QueryBeatmapSets(_ => true).First().DeletePending); + + waitForOrAssert(() => !File.Exists(temp), "Temporary file still exists after standard import", 5000); + + host.Exit(); + } + } + [Test] [NonParallelizable] [Ignore("Binding IPC on Appveyor isn't working (port in use). Need to figure out why")] diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index 8b8a8e197a..049be49e44 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -141,7 +141,7 @@ namespace osu.Game.Beatmaps /// This will post a notification tracking import progress. /// /// One or more beatmap locations on disk. - public void Import(params string[] paths) + public List Import(params string[] paths) { var notification = new ProgressNotification { @@ -153,18 +153,20 @@ namespace osu.Game.Beatmaps PostNotification?.Invoke(notification); + List imported = new List(); + int i = 0; foreach (string path in paths) { if (notification.State == ProgressNotificationState.Cancelled) // user requested abort - return; + return imported; try { notification.Text = $"Importing ({i} of {paths.Length})\n{Path.GetFileName(path)}"; using (ArchiveReader reader = getReaderFrom(path)) - Import(reader); + imported.Add(Import(reader)); notification.Progress = (float)++i / paths.Length; @@ -191,6 +193,7 @@ namespace osu.Game.Beatmaps } notification.State = ProgressNotificationState.Completed; + return imported; } private readonly object importContextLock = new object(); From 8140ffea15830580914d7bcbbdf5444407d5ac8e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 9 Feb 2018 17:58:53 +0900 Subject: [PATCH 03/33] Add test for deleting then reimporting --- .../Beatmaps/IO/ImportBeatmapTest.cs | 48 +++++++++++++++++-- 1 file changed, 45 insertions(+), 3 deletions(-) diff --git a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs index a7ff308c6b..581d787242 100644 --- a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs +++ b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs @@ -71,6 +71,45 @@ namespace osu.Game.Tests.Beatmaps.IO } } + [Test] + public void TestImportThenDeleteThenImport() + { + //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. + using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportThenDeleteThenImport")) + { + var osu = loadOsu(host); + + var temp = prepareTempCopy(osz_path); + Assert.IsTrue(File.Exists(temp)); + + var manager = osu.Dependencies.Get(); + + var imported = manager.Import(temp); + + ensureLoaded(osu); + + manager.Delete(imported.First()); + + Assert.IsTrue(manager.GetAllUsableBeatmapSets().Count == 0); + Assert.IsTrue(manager.QueryBeatmapSets(_ => true).ToList().Count() == 1); + Assert.IsTrue(manager.QueryBeatmapSets(_ => true).First().DeletePending); + + temp = prepareTempCopy(osz_path); + Assert.IsTrue(File.Exists(temp)); + var importedSecondTime = manager.Import(temp); + + ensureLoaded(osu); + + // check the newly "imported" beatmap is actually just the restored previous import. since it matches hash. + Assert.IsTrue(imported.First().ID == importedSecondTime.First().ID); + Assert.IsTrue(imported.First().Beatmaps.First().ID == importedSecondTime.First().Beatmaps.First().ID); + + waitForOrAssert(() => !File.Exists(temp), "Temporary file still exists after standard import", 5000); + + host.Exit(); + } + } + [Test] [NonParallelizable] [Ignore("Binding IPC on Appveyor isn't working (port in use). Need to figure out why")] @@ -166,8 +205,8 @@ namespace osu.Game.Tests.Beatmaps.IO int countBeatmaps = 0; waitForOrAssert(() => - (countBeatmapSetBeatmaps = queryBeatmapSets().First().Beatmaps.Count) == - (countBeatmaps = queryBeatmaps().Count()), + (countBeatmapSetBeatmaps = queryBeatmapSets().First().Beatmaps.Count) == + (countBeatmaps = queryBeatmaps().Count()), $@"Incorrect database beatmap count post-import ({countBeatmaps} but should be {countBeatmapSetBeatmaps}).", timeout); var set = queryBeatmapSets().First(); @@ -192,7 +231,10 @@ namespace osu.Game.Tests.Beatmaps.IO private void waitForOrAssert(Func result, string failureMessage, int timeout = 60000) { - Action waitAction = () => { while (!result()) Thread.Sleep(200); }; + Action waitAction = () => + { + while (!result()) Thread.Sleep(200); + }; Assert.IsTrue(waitAction.BeginInvoke(null, null).AsyncWaitHandle.WaitOne(timeout), failureMessage); } } From a771ca4077eadc196cb810c32bd1b82608c57548 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 9 Feb 2018 18:02:06 +0900 Subject: [PATCH 04/33] Add try-finally to ensure host is exited --- .../Beatmaps/IO/ImportBeatmapTest.cs | 172 +++++++++--------- 1 file changed, 89 insertions(+), 83 deletions(-) diff --git a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs index 581d787242..591ad1680e 100644 --- a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs +++ b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs @@ -26,19 +26,24 @@ namespace osu.Game.Tests.Beatmaps.IO //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportWhenClosed")) { - var osu = loadOsu(host); + try + { + var osu = loadOsu(host); - var temp = prepareTempCopy(osz_path); + var temp = prepareTempCopy(osz_path); - Assert.IsTrue(File.Exists(temp)); + Assert.IsTrue(File.Exists(temp)); - osu.Dependencies.Get().Import(temp); + osu.Dependencies.Get().Import(temp); - ensureLoaded(osu); + ensureLoaded(osu); - waitForOrAssert(() => !File.Exists(temp), "Temporary file still exists after standard import", 5000); - - host.Exit(); + waitForOrAssert(() => !File.Exists(temp), "Temporary file still exists after standard import", 5000); + } + finally + { + host.Exit(); + } } } @@ -48,26 +53,31 @@ namespace osu.Game.Tests.Beatmaps.IO //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportThenDelete")) { - var osu = loadOsu(host); + try + { + var osu = loadOsu(host); - var temp = prepareTempCopy(osz_path); - Assert.IsTrue(File.Exists(temp)); + var temp = prepareTempCopy(osz_path); + Assert.IsTrue(File.Exists(temp)); - var manager = osu.Dependencies.Get(); + var manager = osu.Dependencies.Get(); - var imported = manager.Import(temp); + var imported = manager.Import(temp); - ensureLoaded(osu); + ensureLoaded(osu); - manager.Delete(imported.First()); + manager.Delete(imported.First()); - Assert.IsTrue(manager.GetAllUsableBeatmapSets().Count == 0); - Assert.IsTrue(manager.QueryBeatmapSets(_ => true).ToList().Count() == 1); - Assert.IsTrue(manager.QueryBeatmapSets(_ => true).First().DeletePending); + Assert.IsTrue(manager.GetAllUsableBeatmapSets().Count == 0); + Assert.IsTrue(manager.QueryBeatmapSets(_ => true).ToList().Count == 1); + Assert.IsTrue(manager.QueryBeatmapSets(_ => true).First().DeletePending); - waitForOrAssert(() => !File.Exists(temp), "Temporary file still exists after standard import", 5000); - - host.Exit(); + waitForOrAssert(() => !File.Exists(temp), "Temporary file still exists after standard import", 5000); + } + finally + { + host.Exit(); + } } } @@ -77,36 +87,41 @@ namespace osu.Game.Tests.Beatmaps.IO //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportThenDeleteThenImport")) { - var osu = loadOsu(host); + try + { + var osu = loadOsu(host); - var temp = prepareTempCopy(osz_path); - Assert.IsTrue(File.Exists(temp)); + var temp = prepareTempCopy(osz_path); + Assert.IsTrue(File.Exists(temp)); - var manager = osu.Dependencies.Get(); + var manager = osu.Dependencies.Get(); - var imported = manager.Import(temp); + var imported = manager.Import(temp); - ensureLoaded(osu); + ensureLoaded(osu); - manager.Delete(imported.First()); + manager.Delete(imported.First()); - Assert.IsTrue(manager.GetAllUsableBeatmapSets().Count == 0); - Assert.IsTrue(manager.QueryBeatmapSets(_ => true).ToList().Count() == 1); - Assert.IsTrue(manager.QueryBeatmapSets(_ => true).First().DeletePending); + Assert.IsTrue(manager.GetAllUsableBeatmapSets().Count == 0); + Assert.IsTrue(manager.QueryBeatmapSets(_ => true).ToList().Count() == 1); + Assert.IsTrue(manager.QueryBeatmapSets(_ => true).First().DeletePending); - temp = prepareTempCopy(osz_path); - Assert.IsTrue(File.Exists(temp)); - var importedSecondTime = manager.Import(temp); + temp = prepareTempCopy(osz_path); + Assert.IsTrue(File.Exists(temp)); + var importedSecondTime = manager.Import(temp); - ensureLoaded(osu); + ensureLoaded(osu); - // check the newly "imported" beatmap is actually just the restored previous import. since it matches hash. - Assert.IsTrue(imported.First().ID == importedSecondTime.First().ID); - Assert.IsTrue(imported.First().Beatmaps.First().ID == importedSecondTime.First().Beatmaps.First().ID); + // check the newly "imported" beatmap is actually just the restored previous import. since it matches hash. + Assert.IsTrue(imported.First().ID == importedSecondTime.First().ID); + Assert.IsTrue(imported.First().Beatmaps.First().ID == importedSecondTime.First().Beatmaps.First().ID); - waitForOrAssert(() => !File.Exists(temp), "Temporary file still exists after standard import", 5000); - - host.Exit(); + waitForOrAssert(() => !File.Exists(temp), "Temporary file still exists after standard import", 5000); + } + finally + { + host.Exit(); + } } } @@ -118,24 +133,29 @@ namespace osu.Game.Tests.Beatmaps.IO using (HeadlessGameHost host = new CleanRunHeadlessGameHost("host", true)) using (HeadlessGameHost client = new CleanRunHeadlessGameHost("client", true)) { - Assert.IsTrue(host.IsPrimaryInstance); - Assert.IsFalse(client.IsPrimaryInstance); + try + { + Assert.IsTrue(host.IsPrimaryInstance); + Assert.IsFalse(client.IsPrimaryInstance); - var osu = loadOsu(host); + var osu = loadOsu(host); - var temp = prepareTempCopy(osz_path); + var temp = prepareTempCopy(osz_path); - Assert.IsTrue(File.Exists(temp)); + Assert.IsTrue(File.Exists(temp)); - var importer = new BeatmapIPCChannel(client); - if (!importer.ImportAsync(temp).Wait(10000)) - Assert.Fail(@"IPC took too long to send"); + var importer = new BeatmapIPCChannel(client); + if (!importer.ImportAsync(temp).Wait(10000)) + Assert.Fail(@"IPC took too long to send"); - ensureLoaded(osu); + ensureLoaded(osu); - waitForOrAssert(() => !File.Exists(temp), "Temporary still exists after IPC import", 5000); - - host.Exit(); + waitForOrAssert(() => !File.Exists(temp), "Temporary still exists after IPC import", 5000); + } + finally + { + host.Exit(); + } } } @@ -144,22 +164,21 @@ namespace osu.Game.Tests.Beatmaps.IO { using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportWhenFileOpen")) { - var osu = loadOsu(host); - - var temp = prepareTempCopy(osz_path); - - Assert.IsTrue(File.Exists(temp), "Temporary file copy never substantiated"); - - using (File.OpenRead(temp)) - osu.Dependencies.Get().Import(temp); - - ensureLoaded(osu); - - File.Delete(temp); - - Assert.IsFalse(File.Exists(temp), "We likely held a read lock on the file when we shouldn't"); - - host.Exit(); + try + { + var osu = loadOsu(host); + var temp = prepareTempCopy(osz_path); + Assert.IsTrue(File.Exists(temp), "Temporary file copy never substantiated"); + using (File.OpenRead(temp)) + osu.Dependencies.Get().Import(temp); + ensureLoaded(osu); + File.Delete(temp); + Assert.IsFalse(File.Exists(temp), "We likely held a read lock on the file when we shouldn't"); + } + finally + { + host.Exit(); + } } } @@ -173,58 +192,44 @@ namespace osu.Game.Tests.Beatmaps.IO { var osu = new OsuGameBase(); Task.Run(() => host.Run(osu)); - waitForOrAssert(() => osu.IsLoaded, @"osu! failed to start in a reasonable amount of time"); - return osu; } private void ensureLoaded(OsuGameBase osu, int timeout = 60000) { IEnumerable resultSets = null; - var store = osu.Dependencies.Get(); - waitForOrAssert(() => (resultSets = store.QueryBeatmapSets(s => s.OnlineBeatmapSetID == 241526)).Any(), @"BeatmapSet did not import to the database in allocated time.", timeout); //ensure we were stored to beatmap database backing... Assert.IsTrue(resultSets.Count() == 1, $@"Incorrect result count found ({resultSets.Count()} but should be 1)."); - IEnumerable queryBeatmaps() => store.QueryBeatmaps(s => s.BeatmapSet.OnlineBeatmapSetID == 241526 && s.BaseDifficultyID > 0); IEnumerable queryBeatmapSets() => store.QueryBeatmapSets(s => s.OnlineBeatmapSetID == 241526); //if we don't re-check here, the set will be inserted but the beatmaps won't be present yet. waitForOrAssert(() => queryBeatmaps().Count() == 12, @"Beatmaps did not import to the database in allocated time", timeout); - waitForOrAssert(() => queryBeatmapSets().Count() == 1, @"BeatmapSet did not import to the database in allocated time", timeout); - int countBeatmapSetBeatmaps = 0; int countBeatmaps = 0; - waitForOrAssert(() => (countBeatmapSetBeatmaps = queryBeatmapSets().First().Beatmaps.Count) == (countBeatmaps = queryBeatmaps().Count()), $@"Incorrect database beatmap count post-import ({countBeatmaps} but should be {countBeatmapSetBeatmaps}).", timeout); var set = queryBeatmapSets().First(); - foreach (BeatmapInfo b in set.Beatmaps) Assert.IsTrue(set.Beatmaps.Any(c => c.OnlineBeatmapID == b.OnlineBeatmapID)); - Assert.IsTrue(set.Beatmaps.Count > 0); - var beatmap = store.GetWorkingBeatmap(set.Beatmaps.First(b => b.RulesetID == 0))?.Beatmap; Assert.IsTrue(beatmap?.HitObjects.Count > 0); - beatmap = store.GetWorkingBeatmap(set.Beatmaps.First(b => b.RulesetID == 1))?.Beatmap; Assert.IsTrue(beatmap?.HitObjects.Count > 0); - beatmap = store.GetWorkingBeatmap(set.Beatmaps.First(b => b.RulesetID == 2))?.Beatmap; Assert.IsTrue(beatmap?.HitObjects.Count > 0); - beatmap = store.GetWorkingBeatmap(set.Beatmaps.First(b => b.RulesetID == 3))?.Beatmap; Assert.IsTrue(beatmap?.HitObjects.Count > 0); } @@ -235,6 +240,7 @@ namespace osu.Game.Tests.Beatmaps.IO { while (!result()) Thread.Sleep(200); }; + Assert.IsTrue(waitAction.BeginInvoke(null, null).AsyncWaitHandle.WaitOne(timeout), failureMessage); } } From 981fa379b7d61072e9b07597de11c21728343758 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 9 Feb 2018 18:02:28 +0900 Subject: [PATCH 05/33] Count() -> Count --- osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs index 591ad1680e..7183afb70e 100644 --- a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs +++ b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs @@ -103,7 +103,7 @@ namespace osu.Game.Tests.Beatmaps.IO manager.Delete(imported.First()); Assert.IsTrue(manager.GetAllUsableBeatmapSets().Count == 0); - Assert.IsTrue(manager.QueryBeatmapSets(_ => true).ToList().Count() == 1); + Assert.IsTrue(manager.QueryBeatmapSets(_ => true).ToList().Count == 1); Assert.IsTrue(manager.QueryBeatmapSets(_ => true).First().DeletePending); temp = prepareTempCopy(osz_path); From 623ba652ed003a4d323620317077ac8857fe0ed0 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 9 Feb 2018 18:20:18 +0900 Subject: [PATCH 06/33] Share more code between tests --- .../Beatmaps/IO/ImportBeatmapTest.cs | 82 ++++++++----------- 1 file changed, 33 insertions(+), 49 deletions(-) diff --git a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs index 7183afb70e..0438229252 100644 --- a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs +++ b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs @@ -28,17 +28,7 @@ namespace osu.Game.Tests.Beatmaps.IO { try { - var osu = loadOsu(host); - - var temp = prepareTempCopy(osz_path); - - Assert.IsTrue(File.Exists(temp)); - - osu.Dependencies.Get().Import(temp); - - ensureLoaded(osu); - - waitForOrAssert(() => !File.Exists(temp), "Temporary file still exists after standard import", 5000); + loadOszIntoOsu(loadOsu(host)); } finally { @@ -57,22 +47,9 @@ namespace osu.Game.Tests.Beatmaps.IO { var osu = loadOsu(host); - var temp = prepareTempCopy(osz_path); - Assert.IsTrue(File.Exists(temp)); + var imported = loadOszIntoOsu(osu); - var manager = osu.Dependencies.Get(); - - var imported = manager.Import(temp); - - ensureLoaded(osu); - - manager.Delete(imported.First()); - - Assert.IsTrue(manager.GetAllUsableBeatmapSets().Count == 0); - Assert.IsTrue(manager.QueryBeatmapSets(_ => true).ToList().Count == 1); - Assert.IsTrue(manager.QueryBeatmapSets(_ => true).First().DeletePending); - - waitForOrAssert(() => !File.Exists(temp), "Temporary file still exists after standard import", 5000); + deleteBeatmapSet(imported, osu); } finally { @@ -91,32 +68,15 @@ namespace osu.Game.Tests.Beatmaps.IO { var osu = loadOsu(host); - var temp = prepareTempCopy(osz_path); - Assert.IsTrue(File.Exists(temp)); + var imported = loadOszIntoOsu(osu); - var manager = osu.Dependencies.Get(); + deleteBeatmapSet(imported, osu); - var imported = manager.Import(temp); - - ensureLoaded(osu); - - manager.Delete(imported.First()); - - Assert.IsTrue(manager.GetAllUsableBeatmapSets().Count == 0); - Assert.IsTrue(manager.QueryBeatmapSets(_ => true).ToList().Count == 1); - Assert.IsTrue(manager.QueryBeatmapSets(_ => true).First().DeletePending); - - temp = prepareTempCopy(osz_path); - Assert.IsTrue(File.Exists(temp)); - var importedSecondTime = manager.Import(temp); - - ensureLoaded(osu); + var importedSecondTime = loadOszIntoOsu(osu); // check the newly "imported" beatmap is actually just the restored previous import. since it matches hash. - Assert.IsTrue(imported.First().ID == importedSecondTime.First().ID); - Assert.IsTrue(imported.First().Beatmaps.First().ID == importedSecondTime.First().Beatmaps.First().ID); - - waitForOrAssert(() => !File.Exists(temp), "Temporary file still exists after standard import", 5000); + Assert.IsTrue(imported.ID == importedSecondTime.ID); + Assert.IsTrue(imported.Beatmaps.First().ID == importedSecondTime.Beatmaps.First().ID); } finally { @@ -141,7 +101,6 @@ namespace osu.Game.Tests.Beatmaps.IO var osu = loadOsu(host); var temp = prepareTempCopy(osz_path); - Assert.IsTrue(File.Exists(temp)); var importer = new BeatmapIPCChannel(client); @@ -182,6 +141,31 @@ namespace osu.Game.Tests.Beatmaps.IO } } + private BeatmapSetInfo loadOszIntoOsu(OsuGameBase osu) + { + var temp = prepareTempCopy(osz_path); + + Assert.IsTrue(File.Exists(temp)); + + var imported = osu.Dependencies.Get().Import(temp); + + ensureLoaded(osu); + + waitForOrAssert(() => !File.Exists(temp), "Temporary file still exists after standard import", 5000); + + return imported.FirstOrDefault(); + } + + private void deleteBeatmapSet(BeatmapSetInfo imported, OsuGameBase osu) + { + var manager = osu.Dependencies.Get(); + manager.Delete(imported); + + Assert.IsTrue(manager.GetAllUsableBeatmapSets().Count == 0); + Assert.IsTrue(manager.QueryBeatmapSets(_ => true).ToList().Count == 1); + Assert.IsTrue(manager.QueryBeatmapSets(_ => true).First().DeletePending); + } + private string prepareTempCopy(string path) { var temp = Path.GetTempFileName(); From 541068235d212056731b4e7337f6a8f3e6662753 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 9 Feb 2018 18:23:06 +0900 Subject: [PATCH 07/33] Test import twice in a row --- .../Beatmaps/IO/ImportBeatmapTest.cs | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs index 0438229252..1e97dfefa4 100644 --- a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs +++ b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs @@ -58,6 +58,35 @@ namespace osu.Game.Tests.Beatmaps.IO } } + [Test] + public void TestImportThenImport() + { + //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. + using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportThenDeleteThenImport")) + { + try + { + var osu = loadOsu(host); + + var imported = loadOszIntoOsu(osu); + var importedSecondTime = loadOszIntoOsu(osu); + + // check the newly "imported" beatmap is actually just the restored previous import. since it matches hash. + Assert.IsTrue(imported.ID == importedSecondTime.ID); + Assert.IsTrue(imported.Beatmaps.First().ID == importedSecondTime.Beatmaps.First().ID); + + var manager = osu.Dependencies.Get(); + + Assert.IsTrue(manager.GetAllUsableBeatmapSets().Count == 1); + Assert.IsTrue(manager.QueryBeatmapSets(_ => true).ToList().Count == 1); + } + finally + { + host.Exit(); + } + } + } + [Test] public void TestImportThenDeleteThenImport() { From 5e0cb9d4b9e5cc6d752da0ff7ad317fd6ba72b40 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 9 Feb 2018 19:12:29 +0900 Subject: [PATCH 08/33] Simplify beatmap store retrieval --- osu.Game/Beatmaps/BeatmapManager.cs | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index 049be49e44..63d9874d53 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -67,7 +67,9 @@ namespace osu.Game.Beatmaps private readonly Storage storage; - private BeatmapStore createBeatmapStore(Func context) + private BeatmapStore getBeatmapStoreWithContext(OsuDbContext context) => getBeatmapStoreWithContext(() => context); + + private BeatmapStore getBeatmapStoreWithContext(Func context) { var store = new BeatmapStore(context); store.BeatmapSetAdded += s => BeatmapSetAdded?.Invoke(s); @@ -123,7 +125,7 @@ namespace osu.Game.Beatmaps refreshImportContext(); - beatmaps = createBeatmapStore(context); + beatmaps = getBeatmapStoreWithContext(context); files = new FileStore(context, storage); this.storage = files.Storage; @@ -368,14 +370,10 @@ namespace osu.Game.Beatmaps // re-fetch the beatmap set on the import context. beatmapSet = context.BeatmapSetInfo.Include(s => s.Files).ThenInclude(f => f.FileInfo).First(s => s.ID == beatmapSet.ID); - // create local stores so we can isolate and thread safely, and share a context/transaction. - var iFiles = new FileStore(() => context, storage); - var iBeatmaps = createBeatmapStore(() => context); - - if (iBeatmaps.Delete(beatmapSet)) + if (getBeatmapStoreWithContext(context).Delete(beatmapSet)) { if (!beatmapSet.Protected) - iFiles.Dereference(beatmapSet.Files.Select(f => f.FileInfo).ToArray()); + new FileStore(() => context, storage).Dereference(beatmapSet.Files.Select(f => f.FileInfo).ToArray()); } context.ChangeTracker.AutoDetectChangesEnabled = true; @@ -428,10 +426,7 @@ namespace osu.Game.Beatmaps { context.ChangeTracker.AutoDetectChangesEnabled = false; - var iFiles = new FileStore(() => context, storage); - var iBeatmaps = createBeatmapStore(() => context); - - undelete(iBeatmaps, iFiles, beatmapSet); + undelete(getBeatmapStoreWithContext(context), new FileStore(() => context, storage), beatmapSet); context.ChangeTracker.AutoDetectChangesEnabled = true; context.SaveChanges(transaction); @@ -522,7 +517,7 @@ namespace osu.Game.Beatmaps /// Results from the provided query. public IEnumerable QueryBeatmaps(Expression> query) => beatmaps.Beatmaps.AsNoTracking().Where(query); - private void import(BeatmapSetInfo beatmapSet, OsuDbContext context) => createBeatmapStore(() => context).Add(beatmapSet); + private void import(BeatmapSetInfo beatmapSet, OsuDbContext context) => getBeatmapStoreWithContext(context).Add(beatmapSet); /// /// Creates an from a valid storage path. From c7de79caf6a00f9dd5db3de33af93506cc3988cc Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 9 Feb 2018 19:24:17 +0900 Subject: [PATCH 09/33] Remove storage class variable --- osu.Game/Beatmaps/BeatmapManager.cs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index 63d9874d53..08cf5aeff8 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -65,7 +65,7 @@ namespace osu.Game.Beatmaps /// public WorkingBeatmap DefaultBeatmap { private get; set; } - private readonly Storage storage; + private FileStore getFileStoreWithContext(OsuDbContext context) => new FileStore(() => context, files.Storage); private BeatmapStore getBeatmapStoreWithContext(OsuDbContext context) => getBeatmapStoreWithContext(() => context); @@ -128,7 +128,6 @@ namespace osu.Game.Beatmaps beatmaps = getBeatmapStoreWithContext(context); files = new FileStore(context, storage); - this.storage = files.Storage; this.rulesets = rulesets; this.api = api; @@ -233,7 +232,7 @@ namespace osu.Game.Beatmaps Delete(existingOnlineId); } - beatmapSet.Files = createFileInfos(archive, new FileStore(() => context, storage)); + beatmapSet.Files = createFileInfos(archive, getFileStoreWithContext(context)); beatmapSet.Beatmaps = createBeatmapDifficulties(archive); // remove metadata from difficulties where it matches the set @@ -373,7 +372,7 @@ namespace osu.Game.Beatmaps if (getBeatmapStoreWithContext(context).Delete(beatmapSet)) { if (!beatmapSet.Protected) - new FileStore(() => context, storage).Dereference(beatmapSet.Files.Select(f => f.FileInfo).ToArray()); + getFileStoreWithContext(context).Dereference(beatmapSet.Files.Select(f => f.FileInfo).ToArray()); } context.ChangeTracker.AutoDetectChangesEnabled = true; @@ -426,7 +425,7 @@ namespace osu.Game.Beatmaps { context.ChangeTracker.AutoDetectChangesEnabled = false; - undelete(getBeatmapStoreWithContext(context), new FileStore(() => context, storage), beatmapSet); + undelete(getBeatmapStoreWithContext(context), getFileStoreWithContext(context), beatmapSet); context.ChangeTracker.AutoDetectChangesEnabled = true; context.SaveChanges(transaction); @@ -528,7 +527,7 @@ namespace osu.Game.Beatmaps { if (ZipFile.IsZipFile(path)) // ReSharper disable once InconsistentlySynchronizedField - return new OszArchiveReader(storage.GetStream(path)); + return new OszArchiveReader(files.Storage.GetStream(path)); return new LegacyFilesystemReader(path); } From fb6dc922c62534475445e279b8e8ad389fa02cf4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 9 Feb 2018 19:25:55 +0900 Subject: [PATCH 10/33] Reorder file --- osu.Game/Beatmaps/BeatmapManager.cs | 38 ++++++++++++++--------------- 1 file changed, 19 insertions(+), 19 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index 08cf5aeff8..51d4d6cb22 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -65,20 +65,6 @@ namespace osu.Game.Beatmaps /// public WorkingBeatmap DefaultBeatmap { private get; set; } - private FileStore getFileStoreWithContext(OsuDbContext context) => new FileStore(() => context, files.Storage); - - private BeatmapStore getBeatmapStoreWithContext(OsuDbContext context) => getBeatmapStoreWithContext(() => context); - - private BeatmapStore getBeatmapStoreWithContext(Func context) - { - var store = new BeatmapStore(context); - store.BeatmapSetAdded += s => BeatmapSetAdded?.Invoke(s); - store.BeatmapSetRemoved += s => BeatmapSetRemoved?.Invoke(s); - store.BeatmapHidden += b => BeatmapHidden?.Invoke(b); - store.BeatmapRestored += b => BeatmapRestored?.Invoke(b); - return store; - } - private readonly Func createContext; private readonly FileStore files; @@ -495,6 +481,12 @@ namespace osu.Game.Beatmaps /// A fresh instance. public BeatmapSetInfo Refresh(BeatmapSetInfo beatmapSet) => QueryBeatmapSet(s => s.ID == beatmapSet.ID); + /// + /// Returns a list of all usable s. + /// + /// A list of available . + public List GetAllUsableBeatmapSets() => beatmaps.BeatmapSets.Where(s => !s.DeletePending).ToList(); + /// /// Perform a lookup query on available s. /// @@ -621,11 +613,19 @@ namespace osu.Game.Beatmaps return beatmapInfos; } - /// - /// Returns a list of all usable s. - /// - /// A list of available . - public List GetAllUsableBeatmapSets() => beatmaps.BeatmapSets.Where(s => !s.DeletePending).ToList(); + private FileStore getFileStoreWithContext(OsuDbContext context) => new FileStore(() => context, files.Storage); + + private BeatmapStore getBeatmapStoreWithContext(OsuDbContext context) => getBeatmapStoreWithContext(() => context); + + private BeatmapStore getBeatmapStoreWithContext(Func context) + { + var store = new BeatmapStore(context); + store.BeatmapSetAdded += s => BeatmapSetAdded?.Invoke(s); + store.BeatmapSetRemoved += s => BeatmapSetRemoved?.Invoke(s); + store.BeatmapHidden += b => BeatmapHidden?.Invoke(b); + store.BeatmapRestored += b => BeatmapRestored?.Invoke(b); + return store; + } protected class BeatmapManagerWorkingBeatmap : WorkingBeatmap { From db654004b72422d53898c58198c7f39647ed0ab0 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 9 Feb 2018 19:32:18 +0900 Subject: [PATCH 11/33] Move BeatmapManagerWorkingBeatmap to its own file --- osu.Game/Beatmaps/BeatmapManager.cs | 87 +---------------- .../Beatmaps/BeatmapManager_WorkingBeatmap.cs | 95 +++++++++++++++++++ osu.Game/osu.Game.csproj | 1 + 3 files changed, 97 insertions(+), 86 deletions(-) create mode 100644 osu.Game/Beatmaps/BeatmapManager_WorkingBeatmap.cs diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index 51d4d6cb22..7252bad3c4 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -9,31 +9,26 @@ using System.Linq.Expressions; using System.Threading.Tasks; using Ionic.Zip; using Microsoft.EntityFrameworkCore; -using osu.Framework.Audio.Track; using osu.Framework.Extensions; -using osu.Framework.Graphics.Textures; -using osu.Framework.IO.Stores; using osu.Framework.Logging; using osu.Framework.Platform; using osu.Game.Beatmaps.Formats; using osu.Game.Beatmaps.IO; using osu.Game.Database; using osu.Game.Graphics; -using osu.Game.Graphics.Textures; using osu.Game.IO; using osu.Game.IPC; using osu.Game.Online.API; using osu.Game.Online.API.Requests; using osu.Game.Overlays.Notifications; using osu.Game.Rulesets; -using osu.Game.Storyboards; namespace osu.Game.Beatmaps { /// /// Handles the storage and retrieval of Beatmaps/WorkingBeatmaps. /// - public class BeatmapManager + public partial class BeatmapManager { /// /// Fired when a new becomes available in the database. @@ -627,86 +622,6 @@ namespace osu.Game.Beatmaps return store; } - protected class BeatmapManagerWorkingBeatmap : WorkingBeatmap - { - private readonly IResourceStore store; - - public BeatmapManagerWorkingBeatmap(IResourceStore store, BeatmapInfo beatmapInfo) - : base(beatmapInfo) - { - this.store = store; - } - - protected override Beatmap GetBeatmap() - { - try - { - using (var stream = new StreamReader(store.GetStream(getPathForFile(BeatmapInfo.Path)))) - { - Decoder decoder = Decoder.GetDecoder(stream); - return decoder.DecodeBeatmap(stream); - } - } - catch - { - return null; - } - } - - private string getPathForFile(string filename) => BeatmapSetInfo.Files.First(f => string.Equals(f.Filename, filename, StringComparison.InvariantCultureIgnoreCase)).FileInfo.StoragePath; - - protected override Texture GetBackground() - { - if (Metadata?.BackgroundFile == null) - return null; - - try - { - return new LargeTextureStore(new RawTextureLoaderStore(store)).Get(getPathForFile(Metadata.BackgroundFile)); - } - catch - { - return null; - } - } - - protected override Track GetTrack() - { - try - { - var trackData = store.GetStream(getPathForFile(Metadata.AudioFile)); - return trackData == null ? null : new TrackBass(trackData); - } - catch - { - return new TrackVirtual(); - } - } - - protected override Waveform GetWaveform() => new Waveform(store.GetStream(getPathForFile(Metadata.AudioFile))); - - protected override Storyboard GetStoryboard() - { - try - { - using (var beatmap = new StreamReader(store.GetStream(getPathForFile(BeatmapInfo.Path)))) - { - Decoder decoder = Decoder.GetDecoder(beatmap); - - if (BeatmapSetInfo?.StoryboardFile == null) - return decoder.GetStoryboardDecoder().DecodeStoryboard(beatmap); - - using (var storyboard = new StreamReader(store.GetStream(getPathForFile(BeatmapSetInfo.StoryboardFile)))) - return decoder.GetStoryboardDecoder().DecodeStoryboard(beatmap, storyboard); - } - } - catch - { - return new Storyboard(); - } - } - } - public bool StableInstallationAvailable => GetStableStorage?.Invoke() != null; /// diff --git a/osu.Game/Beatmaps/BeatmapManager_WorkingBeatmap.cs b/osu.Game/Beatmaps/BeatmapManager_WorkingBeatmap.cs new file mode 100644 index 0000000000..2fbacca5e2 --- /dev/null +++ b/osu.Game/Beatmaps/BeatmapManager_WorkingBeatmap.cs @@ -0,0 +1,95 @@ +using System; +using System.IO; +using System.Linq; +using osu.Framework.Audio.Track; +using osu.Framework.Graphics.Textures; +using osu.Framework.IO.Stores; +using osu.Game.Beatmaps.Formats; +using osu.Game.Graphics.Textures; +using osu.Game.Storyboards; + +namespace osu.Game.Beatmaps +{ + public partial class BeatmapManager + { + protected class BeatmapManagerWorkingBeatmap : WorkingBeatmap + { + private readonly IResourceStore store; + + public BeatmapManagerWorkingBeatmap(IResourceStore store, BeatmapInfo beatmapInfo) + : base(beatmapInfo) + { + this.store = store; + } + + protected override Beatmap GetBeatmap() + { + try + { + using (var stream = new StreamReader(store.GetStream(getPathForFile(BeatmapInfo.Path)))) + { + Decoder decoder = Decoder.GetDecoder(stream); + return decoder.DecodeBeatmap(stream); + } + } + catch + { + return null; + } + } + + private string getPathForFile(string filename) => BeatmapSetInfo.Files.First(f => string.Equals(f.Filename, filename, StringComparison.InvariantCultureIgnoreCase)).FileInfo.StoragePath; + + protected override Texture GetBackground() + { + if (Metadata?.BackgroundFile == null) + return null; + + try + { + return new LargeTextureStore(new RawTextureLoaderStore(store)).Get(getPathForFile(Metadata.BackgroundFile)); + } + catch + { + return null; + } + } + + protected override Track GetTrack() + { + try + { + var trackData = store.GetStream(getPathForFile(Metadata.AudioFile)); + return trackData == null ? null : new TrackBass(trackData); + } + catch + { + return new TrackVirtual(); + } + } + + protected override Waveform GetWaveform() => new Waveform(store.GetStream(getPathForFile(Metadata.AudioFile))); + + protected override Storyboard GetStoryboard() + { + try + { + using (var beatmap = new StreamReader(store.GetStream(getPathForFile(BeatmapInfo.Path)))) + { + Decoder decoder = Decoder.GetDecoder(beatmap); + + if (BeatmapSetInfo?.StoryboardFile == null) + return decoder.GetStoryboardDecoder().DecodeStoryboard(beatmap); + + using (var storyboard = new StreamReader(store.GetStream(getPathForFile(BeatmapSetInfo.StoryboardFile)))) + return decoder.GetStoryboardDecoder().DecodeStoryboard(beatmap, storyboard); + } + } + catch + { + return new Storyboard(); + } + } + } + } +} diff --git a/osu.Game/osu.Game.csproj b/osu.Game/osu.Game.csproj index 6542160b97..c16767c02c 100644 --- a/osu.Game/osu.Game.csproj +++ b/osu.Game/osu.Game.csproj @@ -248,6 +248,7 @@ + From c84cb0b33c215f011b6836c1175246025f5937b8 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 9 Feb 2018 19:32:28 +0900 Subject: [PATCH 12/33] Fix/add some xmldoc --- osu.Game/Beatmaps/BeatmapManager.cs | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index 7252bad3c4..143ae81fa6 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -503,6 +503,9 @@ namespace osu.Game.Beatmaps /// Results from the provided query. public IEnumerable QueryBeatmaps(Expression> query) => beatmaps.Beatmaps.AsNoTracking().Where(query); + /// + /// Import a into the beatmap store. + /// private void import(BeatmapSetInfo beatmapSet, OsuDbContext context) => getBeatmapStoreWithContext(context).Add(beatmapSet); /// @@ -530,10 +533,8 @@ namespace osu.Game.Beatmaps } /// - /// + /// Create a from a provided archive. /// - /// - /// private BeatmapSetInfo createBeatmapSetInfo(ArchiveReader reader) { // let's make sure there are actually .osu files to import. @@ -553,6 +554,9 @@ namespace osu.Game.Beatmaps }; } + /// + /// Create all required s for the provided archive, adding them to the global file store. + /// private List createFileInfos(ArchiveReader reader, FileStore files) { List fileInfos = new List(); From 867b1b5f65889b483c1be93501a3d0bc1be5c707 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 9 Feb 2018 19:33:10 +0900 Subject: [PATCH 13/33] Move public methods up --- osu.Game/Beatmaps/BeatmapManager.cs | 99 ++++++++++++++--------------- 1 file changed, 49 insertions(+), 50 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index 143ae81fa6..bb3a23548a 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -230,7 +230,6 @@ namespace osu.Game.Beatmaps } } - /// /// Import a beatmap from a . /// @@ -503,6 +502,55 @@ namespace osu.Game.Beatmaps /// Results from the provided query. public IEnumerable QueryBeatmaps(Expression> query) => beatmaps.Beatmaps.AsNoTracking().Where(query); + public bool StableInstallationAvailable => GetStableStorage?.Invoke() != null; + + /// + /// This is a temporary method and will likely be replaced by a full-fledged (and more correctly placed) migration process in the future. + /// + public async Task ImportFromStable() + { + var stable = GetStableStorage?.Invoke(); + + if (stable == null) + { + Logger.Log("No osu!stable installation available!", LoggingTarget.Information, LogLevel.Error); + return; + } + + await Task.Factory.StartNew(() => Import(stable.GetDirectories("Songs")), TaskCreationOptions.LongRunning); + } + + public void DeleteAll() + { + var maps = GetAllUsableBeatmapSets(); + + if (maps.Count == 0) return; + + var notification = new ProgressNotification + { + Progress = 0, + CompletionText = "Deleted all beatmaps!", + State = ProgressNotificationState.Active, + }; + + PostNotification?.Invoke(notification); + + int i = 0; + + foreach (var b in maps) + { + if (notification.State == ProgressNotificationState.Cancelled) + // user requested abort + return; + + notification.Text = $"Deleting ({i} of {maps.Count})"; + notification.Progress = (float)++i / maps.Count; + Delete(b); + } + + notification.State = ProgressNotificationState.Completed; + } + /// /// Import a into the beatmap store. /// @@ -625,54 +673,5 @@ namespace osu.Game.Beatmaps store.BeatmapRestored += b => BeatmapRestored?.Invoke(b); return store; } - - public bool StableInstallationAvailable => GetStableStorage?.Invoke() != null; - - /// - /// This is a temporary method and will likely be replaced by a full-fledged (and more correctly placed) migration process in the future. - /// - public async Task ImportFromStable() - { - var stable = GetStableStorage?.Invoke(); - - if (stable == null) - { - Logger.Log("No osu!stable installation available!", LoggingTarget.Information, LogLevel.Error); - return; - } - - await Task.Factory.StartNew(() => Import(stable.GetDirectories("Songs")), TaskCreationOptions.LongRunning); - } - - public void DeleteAll() - { - var maps = GetAllUsableBeatmapSets(); - - if (maps.Count == 0) return; - - var notification = new ProgressNotification - { - Progress = 0, - CompletionText = "Deleted all beatmaps!", - State = ProgressNotificationState.Active, - }; - - PostNotification?.Invoke(notification); - - int i = 0; - - foreach (var b in maps) - { - if (notification.State == ProgressNotificationState.Cancelled) - // user requested abort - return; - - notification.Text = $"Deleting ({i} of {maps.Count})"; - notification.Progress = (float)++i / maps.Count; - Delete(b); - } - - notification.State = ProgressNotificationState.Completed; - } } } From d547caa04ef88b0deaf8f56320ed65381c02b6e9 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 9 Feb 2018 19:44:17 +0900 Subject: [PATCH 14/33] Further improve xmldoc --- osu.Game/Beatmaps/BeatmapManager.cs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index bb3a23548a..c0a5a5b39b 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -120,7 +120,7 @@ namespace osu.Game.Beatmaps /// /// Import one or more from filesystem . - /// This will post a notification tracking import progress. + /// This will post notifications tracking progress. /// /// One or more beatmap locations on disk. public List Import(params string[] paths) @@ -250,6 +250,7 @@ namespace osu.Game.Beatmaps /// /// Downloads a beatmap. + /// This will post notifications tracking progress. /// /// The to be downloaded. /// Whether the beatmap should be downloaded without video. Defaults to false. @@ -361,6 +362,10 @@ namespace osu.Game.Beatmaps } } + /// + /// Restore all beatmaps that were previously deleted. + /// This will post notifications tracking progress. + /// public void UndeleteAll() { var deleteMaps = QueryBeatmapSets(bs => bs.DeletePending).ToList(); @@ -392,6 +397,10 @@ namespace osu.Game.Beatmaps notification.State = ProgressNotificationState.Completed; } + /// + /// Restore a beatmap that was previously deleted. Is a no-op if the beatmap is not in a deleted state, or has its protected flag set. + /// + /// The beatmap to restore public void Undelete(BeatmapSetInfo beatmapSet) { if (beatmapSet.Protected) @@ -502,6 +511,9 @@ namespace osu.Game.Beatmaps /// Results from the provided query. public IEnumerable QueryBeatmaps(Expression> query) => beatmaps.Beatmaps.AsNoTracking().Where(query); + /// + /// Denotes whether an osu-stable installation is present to perform automated imports from. + /// public bool StableInstallationAvailable => GetStableStorage?.Invoke() != null; /// @@ -520,6 +532,10 @@ namespace osu.Game.Beatmaps await Task.Factory.StartNew(() => Import(stable.GetDirectories("Songs")), TaskCreationOptions.LongRunning); } + /// + /// Delete all beatmaps. + /// This will post notifications tracking progress. + /// public void DeleteAll() { var maps = GetAllUsableBeatmapSets(); @@ -569,6 +585,9 @@ namespace osu.Game.Beatmaps return new LegacyFilesystemReader(path); } + /// + /// Create a SHA-2 hash from the provided archive based on contained beatmap filenames. + /// private string computeBeatmapSetHash(ArchiveReader reader) { // for now, concatenate all .osu files in the set to create a unique hash. From a1513351c0454b3b060fc83a3e4aac090ae63923 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 9 Feb 2018 20:35:54 +0900 Subject: [PATCH 15/33] Add missing licence header --- osu.Game/Beatmaps/BeatmapManager_WorkingBeatmap.cs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/osu.Game/Beatmaps/BeatmapManager_WorkingBeatmap.cs b/osu.Game/Beatmaps/BeatmapManager_WorkingBeatmap.cs index 2fbacca5e2..14a4028b44 100644 --- a/osu.Game/Beatmaps/BeatmapManager_WorkingBeatmap.cs +++ b/osu.Game/Beatmaps/BeatmapManager_WorkingBeatmap.cs @@ -1,4 +1,7 @@ -using System; +// Copyright (c) 2007-2018 ppy Pty Ltd . +// Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE + +using System; using System.IO; using System.Linq; using osu.Framework.Audio.Track; From 721bb7e4dd25b719128d5e2a33f38156257fe10a Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 9 Feb 2018 21:31:33 +0900 Subject: [PATCH 16/33] Add proper handling for OnlineBeatmapSetID conflicts Not yet working --- .../Beatmaps/IO/ImportBeatmapTest.cs | 34 +++++++++++++++++++ osu.Game/Beatmaps/BeatmapManager.cs | 11 ++++++ osu.Game/Beatmaps/BeatmapStore.cs | 28 ++++++++++++--- 3 files changed, 69 insertions(+), 4 deletions(-) diff --git a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs index 1e97dfefa4..4da9cba446 100644 --- a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs +++ b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs @@ -87,6 +87,40 @@ namespace osu.Game.Tests.Beatmaps.IO } } + [Test] + public void TestImportThenImportDifferentHash() + { + //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. + using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportThenImportDifferentHash")) + { + try + { + var osu = loadOsu(host); + var manager = osu.Dependencies.Get(); + + var imported = loadOszIntoOsu(osu); + + //var change = manager.QueryBeatmapSets(_ => true).First(); + imported.Hash += "-changed"; + manager.Update(imported); + + var importedSecondTime = loadOszIntoOsu(osu); + + // check the newly "imported" beatmap is actually just the restored previous import. since it matches hash. + Assert.IsTrue(imported.ID == importedSecondTime.ID); + Assert.IsTrue(imported.Beatmaps.First().ID == importedSecondTime.Beatmaps.First().ID); + + + Assert.IsTrue(manager.GetAllUsableBeatmapSets().Count == 1); + Assert.IsTrue(manager.QueryBeatmapSets(_ => true).ToList().Count == 1); + } + finally + { + host.Exit(); + } + } + } + [Test] public void TestImportThenDeleteThenImport() { diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index c0a5a5b39b..cbaa8a1066 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -210,7 +210,12 @@ namespace osu.Game.Beatmaps { var existingOnlineId = beatmaps.BeatmapSets.FirstOrDefault(b => b.OnlineBeatmapSetID == beatmapSet.OnlineBeatmapSetID); if (existingOnlineId != null) + { + // {Microsoft.EntityFrameworkCore.DbUpdateConcurrencyException: Database operation expected to affect 1 row(s) but actually affected 0 row(s). Data may have been modified or deleted since entities were loaded. See http://go.microsoft.com/fwlink/?LinkId=527962…} + Delete(existingOnlineId); + beatmaps.Cleanup(s => s.ID == existingOnlineId.ID); + } } beatmapSet.Files = createFileInfos(archive, getFileStoreWithContext(context)); @@ -332,6 +337,12 @@ namespace osu.Game.Beatmaps /// The object if it exists, or null. public DownloadBeatmapSetRequest GetExistingDownload(BeatmapSetInfo beatmap) => currentDownloads.Find(d => d.BeatmapSet.OnlineBeatmapSetID == beatmap.OnlineBeatmapSetID); + /// + /// Update a BeatmapSetInfo with all changes. TODO: This only supports very basic updates currently. + /// + /// The beatmap set to update. + public void Update(BeatmapSetInfo beatmap) => beatmaps.Update(beatmap); + /// /// Delete a beatmap from the manager. /// Is a no-op for already deleted beatmaps. diff --git a/osu.Game/Beatmaps/BeatmapStore.cs b/osu.Game/Beatmaps/BeatmapStore.cs index df71c5c0d0..f2c3eddec9 100644 --- a/osu.Game/Beatmaps/BeatmapStore.cs +++ b/osu.Game/Beatmaps/BeatmapStore.cs @@ -3,6 +3,7 @@ using System; using System.Linq; +using System.Linq.Expressions; using Microsoft.EntityFrameworkCore; using osu.Game.Database; @@ -50,6 +51,22 @@ namespace osu.Game.Beatmaps BeatmapSetAdded?.Invoke(beatmapSet); } + /// + /// Update a in the database. TODO: This only supports very basic updates currently. + /// + /// The beatmap to update. + public void Update(BeatmapSetInfo beatmapSet) + { + BeatmapSetRemoved?.Invoke(beatmapSet); + + var context = GetContext(); + + context.BeatmapSetInfo.Update(beatmapSet); + context.SaveChanges(); + + BeatmapSetAdded?.Invoke(beatmapSet); + } + /// /// Delete a from the database. /// @@ -126,14 +143,17 @@ namespace osu.Game.Beatmaps return true; } - public override void Cleanup() + public override void Cleanup() => Cleanup(_ => true); + + public void Cleanup(Expression> query) { var context = GetContext(); var purgeable = context.BeatmapSetInfo.Where(s => s.DeletePending && !s.Protected) - .Include(s => s.Beatmaps).ThenInclude(b => b.Metadata) - .Include(s => s.Beatmaps).ThenInclude(b => b.BaseDifficulty) - .Include(s => s.Metadata); + .Where(query) + .Include(s => s.Beatmaps).ThenInclude(b => b.Metadata) + .Include(s => s.Beatmaps).ThenInclude(b => b.BaseDifficulty) + .Include(s => s.Metadata); // metadata is M-N so we can't rely on cascades context.BeatmapMetadata.RemoveRange(purgeable.Select(s => s.Metadata)); From c3ce015869c93b5e948fdbe1879bdeed6ea181f6 Mon Sep 17 00:00:00 2001 From: Aergwyn Date: Sun, 11 Feb 2018 11:03:01 +0100 Subject: [PATCH 17/33] fade slider ticks with hidden mod --- osu.Game.Rulesets.Osu/Mods/OsuModHidden.cs | 15 +++++++++------ .../Objects/Drawables/DrawableSliderTick.cs | 14 +++++++------- 2 files changed, 16 insertions(+), 13 deletions(-) diff --git a/osu.Game.Rulesets.Osu/Mods/OsuModHidden.cs b/osu.Game.Rulesets.Osu/Mods/OsuModHidden.cs index b4dd08eadb..beabeb0a19 100644 --- a/osu.Game.Rulesets.Osu/Mods/OsuModHidden.cs +++ b/osu.Game.Rulesets.Osu/Mods/OsuModHidden.cs @@ -1,6 +1,7 @@ // Copyright (c) 2007-2018 ppy Pty Ltd . // Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE +using System; using System.Collections.Generic; using System.Linq; using osu.Framework.Graphics; @@ -47,16 +48,20 @@ namespace osu.Game.Rulesets.Osu.Mods // fade out immediately after fade in. using (drawable.BeginAbsoluteSequence(fadeOutStartTime, true)) - { circle.FadeOut(fadeOutDuration); - } break; case DrawableSlider slider: using (slider.BeginAbsoluteSequence(fadeOutStartTime, true)) - { slider.Body.FadeOut(longFadeDuration, Easing.Out); - } + + break; + case DrawableSliderTick sliderTick: + // slider ticks fade out over up to one second + var tickFadeOutDuration = Math.Min(sliderTick.HitObject.TimePreempt - DrawableSliderTick.ANIM_DURATION, 1000); + + using (sliderTick.BeginAbsoluteSequence(sliderTick.HitObject.StartTime - tickFadeOutDuration, true)) + sliderTick.FadeOut(tickFadeOutDuration); break; case DrawableSpinner spinner: @@ -66,9 +71,7 @@ namespace osu.Game.Rulesets.Osu.Mods spinner.Background.Hide(); using (spinner.BeginAbsoluteSequence(fadeOutStartTime + longFadeDuration, true)) - { spinner.FadeOut(fadeOutDuration); - } break; } diff --git a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTick.cs b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTick.cs index 41d73a745a..baa9eac1a3 100644 --- a/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTick.cs +++ b/osu.Game.Rulesets.Osu/Objects/Drawables/DrawableSliderTick.cs @@ -13,7 +13,7 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables { public class DrawableSliderTick : DrawableOsuHitObject, IRequireTracking { - private const double anim_duration = 150; + public const double ANIM_DURATION = 150; public bool Tracking { get; set; } @@ -51,8 +51,8 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables protected override void UpdatePreemptState() { this.Animate( - d => d.FadeIn(anim_duration), - d => d.ScaleTo(0.5f).ScaleTo(1f, anim_duration * 4, Easing.OutElasticHalf) + d => d.FadeIn(ANIM_DURATION), + d => d.ScaleTo(0.5f).ScaleTo(1f, ANIM_DURATION * 4, Easing.OutElasticHalf) ); } @@ -64,12 +64,12 @@ namespace osu.Game.Rulesets.Osu.Objects.Drawables this.Delay(HitObject.TimePreempt).FadeOut(); break; case ArmedState.Miss: - this.FadeOut(anim_duration) - .FadeColour(Color4.Red, anim_duration / 2); + this.FadeOut(ANIM_DURATION) + .FadeColour(Color4.Red, ANIM_DURATION / 2); break; case ArmedState.Hit: - this.FadeOut(anim_duration, Easing.OutQuint) - .ScaleTo(Scale * 1.5f, anim_duration, Easing.Out); + this.FadeOut(ANIM_DURATION, Easing.OutQuint) + .ScaleTo(Scale * 1.5f, ANIM_DURATION, Easing.Out); break; } } From efeffaf634bbfe10eb19f354bd973df82bcfaf15 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 12 Feb 2018 11:40:24 +0900 Subject: [PATCH 18/33] Update CFS version --- appveyor.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/appveyor.yml b/appveyor.yml index 9cf68803a2..b86082334d 100644 --- a/appveyor.yml +++ b/appveyor.yml @@ -12,7 +12,7 @@ install: - cmd: git submodule update --init --recursive --depth=5 - cmd: choco install resharper-clt -y - cmd: choco install nvika -y - - cmd: appveyor DownloadFile https://github.com/peppy/CodeFileSanity/releases/download/v0.2.3/CodeFileSanity.exe + - cmd: appveyor DownloadFile https://github.com/peppy/CodeFileSanity/releases/download/v0.2.4/CodeFileSanity.exe before_build: - cmd: CodeFileSanity.exe - cmd: nuget restore -verbosity quiet From 264a0f59e2d91ba963ec7c4a3fd74571b7beb30f Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 12 Feb 2018 12:22:13 +0900 Subject: [PATCH 19/33] Fix duplicate test name --- osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs index 4da9cba446..a0ca60f1f2 100644 --- a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs +++ b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs @@ -62,7 +62,7 @@ namespace osu.Game.Tests.Beatmaps.IO public void TestImportThenImport() { //unfortunately for the time being we need to reference osu.Framework.Desktop for a game host here. - using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportThenDeleteThenImport")) + using (HeadlessGameHost host = new CleanRunHeadlessGameHost("TestImportThenImport")) { try { From e54de0c267a85003a78570c601fd18283c8c00d4 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 12 Feb 2018 15:25:09 +0900 Subject: [PATCH 20/33] Remove sqlite-net migration Anyone that may have benefited from this already has. --- osu.Game/Database/OsuDbContext.cs | 82 ------------------------------- 1 file changed, 82 deletions(-) diff --git a/osu.Game/Database/OsuDbContext.cs b/osu.Game/Database/OsuDbContext.cs index 0fa1f238a9..cf29ae4496 100644 --- a/osu.Game/Database/OsuDbContext.cs +++ b/osu.Game/Database/OsuDbContext.cs @@ -186,8 +186,6 @@ namespace osu.Game.Database public void Migrate() { - migrateFromSqliteNet(); - try { Database.Migrate(); @@ -197,86 +195,6 @@ namespace osu.Game.Database throw new MigrationFailedException(e); } } - - private void migrateFromSqliteNet() - { - try - { - // will fail if the database isn't in a sane EF-migrated state. - Database.ExecuteSqlCommand("SELECT MetadataID FROM BeatmapSetInfo LIMIT 1"); - } - catch - { - try - { - Database.ExecuteSqlCommand("DROP TABLE IF EXISTS __EFMigrationsHistory"); - - // will fail (intentionally) if we don't have sqlite-net data present. - Database.ExecuteSqlCommand("SELECT OnlineBeatmapSetId FROM BeatmapMetadata LIMIT 1"); - - try - { - Logger.Log("Performing migration from sqlite-net to EF...", LoggingTarget.Database, Framework.Logging.LogLevel.Important); - - // we are good to perform messy migration of data!. - Database.ExecuteSqlCommand("ALTER TABLE BeatmapDifficulty RENAME TO BeatmapDifficulty_Old"); - Database.ExecuteSqlCommand("ALTER TABLE BeatmapMetadata RENAME TO BeatmapMetadata_Old"); - Database.ExecuteSqlCommand("ALTER TABLE FileInfo RENAME TO FileInfo_Old"); - Database.ExecuteSqlCommand("ALTER TABLE KeyBinding RENAME TO KeyBinding_Old"); - Database.ExecuteSqlCommand("ALTER TABLE BeatmapSetInfo RENAME TO BeatmapSetInfo_Old"); - Database.ExecuteSqlCommand("ALTER TABLE BeatmapInfo RENAME TO BeatmapInfo_Old"); - Database.ExecuteSqlCommand("ALTER TABLE BeatmapSetFileInfo RENAME TO BeatmapSetFileInfo_Old"); - Database.ExecuteSqlCommand("ALTER TABLE RulesetInfo RENAME TO RulesetInfo_Old"); - - Database.ExecuteSqlCommand("DROP TABLE StoreVersion"); - - // perform EF migrations to create sane table structure. - Database.Migrate(); - - // copy data table by table to new structure, dropping old tables as we go. - Database.ExecuteSqlCommand("INSERT INTO FileInfo SELECT * FROM FileInfo_Old"); - Database.ExecuteSqlCommand("DROP TABLE FileInfo_Old"); - - Database.ExecuteSqlCommand("INSERT INTO KeyBinding SELECT ID, [Action], Keys, RulesetID, Variant FROM KeyBinding_Old"); - Database.ExecuteSqlCommand("DROP TABLE KeyBinding_Old"); - - Database.ExecuteSqlCommand( - "INSERT INTO BeatmapMetadata SELECT ID, Artist, ArtistUnicode, AudioFile, Author, BackgroundFile, PreviewTime, Source, Tags, Title, TitleUnicode FROM BeatmapMetadata_Old"); - Database.ExecuteSqlCommand("DROP TABLE BeatmapMetadata_Old"); - - Database.ExecuteSqlCommand( - "INSERT INTO BeatmapDifficulty SELECT `ID`, `ApproachRate`, `CircleSize`, `DrainRate`, `OverallDifficulty`, `SliderMultiplier`, `SliderTickRate` FROM BeatmapDifficulty_Old"); - Database.ExecuteSqlCommand("DROP TABLE BeatmapDifficulty_Old"); - - Database.ExecuteSqlCommand("INSERT INTO BeatmapSetInfo SELECT ID, DeletePending, Hash, BeatmapMetadataID, OnlineBeatmapSetID, Protected FROM BeatmapSetInfo_Old"); - Database.ExecuteSqlCommand("DROP TABLE BeatmapSetInfo_Old"); - - Database.ExecuteSqlCommand("INSERT INTO BeatmapSetFileInfo SELECT ID, BeatmapSetInfoID, FileInfoID, Filename FROM BeatmapSetFileInfo_Old"); - Database.ExecuteSqlCommand("DROP TABLE BeatmapSetFileInfo_Old"); - - Database.ExecuteSqlCommand("INSERT INTO RulesetInfo SELECT ID, Available, InstantiationInfo, Name FROM RulesetInfo_Old"); - Database.ExecuteSqlCommand("DROP TABLE RulesetInfo_Old"); - - Database.ExecuteSqlCommand( - "INSERT INTO BeatmapInfo SELECT ID, AudioLeadIn, BaseDifficultyID, BeatDivisor, BeatmapSetInfoID, Countdown, DistanceSpacing, GridSize, Hash, IFNULL(Hidden, 0), LetterboxInBreaks, MD5Hash, NULLIF(BeatmapMetadataID, 0), NULLIF(OnlineBeatmapID, 0), Path, RulesetID, SpecialStyle, StackLeniency, StarDifficulty, StoredBookmarks, TimelineZoom, Version, WidescreenStoryboard FROM BeatmapInfo_Old"); - Database.ExecuteSqlCommand("DROP TABLE BeatmapInfo_Old"); - - Logger.Log("Migration complete!", LoggingTarget.Database, Framework.Logging.LogLevel.Important); - } - catch (Exception e) - { - throw new MigrationFailedException(e); - } - } - catch (MigrationFailedException) - { - throw; - } - catch - { - } - } - } } public class MigrationFailedException : Exception From 9ed05543d7161dc0a3d71299db5f1a665bf2f571 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 12 Feb 2018 15:39:00 +0900 Subject: [PATCH 21/33] Fix post-test conditionals from being inverse of what we want to test --- osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs index a0ca60f1f2..cade50a9f3 100644 --- a/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs +++ b/osu.Game.Tests/Beatmaps/IO/ImportBeatmapTest.cs @@ -107,9 +107,8 @@ namespace osu.Game.Tests.Beatmaps.IO var importedSecondTime = loadOszIntoOsu(osu); // check the newly "imported" beatmap is actually just the restored previous import. since it matches hash. - Assert.IsTrue(imported.ID == importedSecondTime.ID); - Assert.IsTrue(imported.Beatmaps.First().ID == importedSecondTime.Beatmaps.First().ID); - + Assert.IsTrue(imported.ID != importedSecondTime.ID); + Assert.IsTrue(imported.Beatmaps.First().ID < importedSecondTime.Beatmaps.First().ID); Assert.IsTrue(manager.GetAllUsableBeatmapSets().Count == 1); Assert.IsTrue(manager.QueryBeatmapSets(_ => true).ToList().Count == 1); From cc948d688f90453e58cc2dcdceb8ca46bd0d338e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 12 Feb 2018 15:39:13 +0900 Subject: [PATCH 22/33] Fix unrelated spacing issue --- osu.Game/OsuGameBase.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/OsuGameBase.cs b/osu.Game/OsuGameBase.cs index 937b204c81..a7eac27056 100644 --- a/osu.Game/OsuGameBase.cs +++ b/osu.Game/OsuGameBase.cs @@ -218,7 +218,7 @@ namespace osu.Game CursorOverrideContainer.Child = globalBinding = new GlobalActionContainer(this) { RelativeSizeAxes = Axes.Both, - Child = content = new OsuTooltipContainer(CursorOverrideContainer.Cursor) { RelativeSizeAxes = Axes.Both　} + Child = content = new OsuTooltipContainer(CursorOverrideContainer.Cursor) { RelativeSizeAxes = Axes.Both } }; base.Content.Add(new DrawSizePreservingFillContainer { Child = CursorOverrideContainer }); From edc36381752067292b9f2faeb3921794ea5e84bb Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 12 Feb 2018 17:55:11 +0900 Subject: [PATCH 23/33] DatabaseWriteUsage --- .../Visual/TestCasePlaySongSelect.cs | 8 +- osu.Game/Beatmaps/BeatmapManager.cs | 188 ++++++------------ osu.Game/Beatmaps/BeatmapStore.cs | 124 ++++++------ osu.Game/Configuration/SettingsStore.cs | 21 +- osu.Game/Database/DatabaseBackedStore.cs | 44 ++-- osu.Game/Database/DatabaseContextFactory.cs | 60 +++++- osu.Game/Database/DatabaseWriteUsage.cs | 28 +++ osu.Game/Database/SingletonContextFactory.cs | 21 ++ osu.Game/IO/FileStore.cs | 109 +++++----- osu.Game/Input/KeyBindingStore.cs | 40 ++-- osu.Game/OsuGameBase.cs | 20 +- osu.Game/Rulesets/RulesetStore.cs | 71 +++---- osu.Game/Rulesets/Scoring/ScoreStore.cs | 3 +- osu.Game/osu.Game.csproj | 2 + 14 files changed, 385 insertions(+), 354 deletions(-) create mode 100644 osu.Game/Database/DatabaseWriteUsage.cs create mode 100644 osu.Game/Database/SingletonContextFactory.cs diff --git a/osu.Game.Tests/Visual/TestCasePlaySongSelect.cs b/osu.Game.Tests/Visual/TestCasePlaySongSelect.cs index 809de2b8db..f54eb77c6b 100644 --- a/osu.Game.Tests/Visual/TestCasePlaySongSelect.cs +++ b/osu.Game.Tests/Visual/TestCasePlaySongSelect.cs @@ -63,12 +63,10 @@ namespace osu.Game.Tests.Visual var storage = new TestStorage(@"TestCasePlaySongSelect"); // this is by no means clean. should be replacing inside of OsuGameBase somehow. - var context = new OsuDbContext(); + DatabaseContextFactory factory = new SingletonContextFactory(new OsuDbContext()); - OsuDbContext contextFactory() => context; - - dependencies.Cache(rulesets = new RulesetStore(contextFactory)); - dependencies.Cache(manager = new BeatmapManager(storage, contextFactory, rulesets, null) + dependencies.Cache(rulesets = new RulesetStore(factory)); + dependencies.Cache(manager = new BeatmapManager(storage, factory, rulesets, null) { DefaultBeatmap = defaultBeatmap = game.Beatmap.Default }); diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index cbaa8a1066..4ec153c78f 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -60,7 +60,7 @@ namespace osu.Game.Beatmaps /// public WorkingBeatmap DefaultBeatmap { private get; set; } - private readonly Func createContext; + private readonly DatabaseContextFactory contextFactory; private readonly FileStore files; @@ -85,29 +85,18 @@ namespace osu.Game.Beatmaps /// public Func GetStableStorage { private get; set; } - private void refreshImportContext() + public BeatmapManager(Storage storage, DatabaseContextFactory contextFactory, RulesetStore rulesets, APIAccess api, IIpcHost importHost = null) { - lock (importContextLock) - { - importContext?.Value?.Dispose(); + this.contextFactory = contextFactory; - importContext = new Lazy(() => - { - var c = createContext(); - c.Database.AutoTransactionsEnabled = false; - return c; - }); - } - } + beatmaps = new BeatmapStore(contextFactory); - public BeatmapManager(Storage storage, Func context, RulesetStore rulesets, APIAccess api, IIpcHost importHost = null) - { - createContext = context; + beatmaps.BeatmapSetAdded += s => BeatmapSetAdded?.Invoke(s); + beatmaps.BeatmapSetRemoved += s => BeatmapSetRemoved?.Invoke(s); + beatmaps.BeatmapHidden += b => BeatmapHidden?.Invoke(b); + beatmaps.BeatmapRestored += b => BeatmapRestored?.Invoke(b); - refreshImportContext(); - - beatmaps = getBeatmapStoreWithContext(context); - files = new FileStore(context, storage); + files = new FileStore(contextFactory, storage); this.rulesets = rulesets; this.api = api; @@ -170,7 +159,6 @@ namespace osu.Game.Beatmaps { e = e.InnerException ?? e; Logger.Error(e, $@"Could not import beatmap set ({Path.GetFileName(path)})"); - refreshImportContext(); } } @@ -178,80 +166,57 @@ namespace osu.Game.Beatmaps return imported; } - private readonly object importContextLock = new object(); - private Lazy importContext; - /// /// Import a beatmap from an . /// /// The beatmap to be imported. public BeatmapSetInfo Import(ArchiveReader archive) { - // let's only allow one concurrent import at a time for now - lock (importContextLock) + using ( contextFactory.GetForWrite()) // used to share a context for full import. keep in mind this will block all writes. { - var context = importContext.Value; + // create a new set info (don't yet add to database) + var beatmapSet = createBeatmapSetInfo(archive); - using (var transaction = context.BeginTransaction()) + // check if this beatmap has already been imported and exit early if so + var existingHashMatch = beatmaps.BeatmapSets.FirstOrDefault(b => b.Hash == beatmapSet.Hash); + if (existingHashMatch != null) { - // create a new set info (don't yet add to database) - var beatmapSet = createBeatmapSetInfo(archive); - - // check if this beatmap has already been imported and exit early if so - var existingHashMatch = beatmaps.BeatmapSets.FirstOrDefault(b => b.Hash == beatmapSet.Hash); - if (existingHashMatch != null) - { - undelete(beatmaps, files, existingHashMatch); - return existingHashMatch; - } - - // check if a set already exists with the same online id - if (beatmapSet.OnlineBeatmapSetID != null) - { - var existingOnlineId = beatmaps.BeatmapSets.FirstOrDefault(b => b.OnlineBeatmapSetID == beatmapSet.OnlineBeatmapSetID); - if (existingOnlineId != null) - { - // {Microsoft.EntityFrameworkCore.DbUpdateConcurrencyException: Database operation expected to affect 1 row(s) but actually affected 0 row(s). Data may have been modified or deleted since entities were loaded. See http://go.microsoft.com/fwlink/?LinkId=527962…} - - Delete(existingOnlineId); - beatmaps.Cleanup(s => s.ID == existingOnlineId.ID); - } - } - - beatmapSet.Files = createFileInfos(archive, getFileStoreWithContext(context)); - beatmapSet.Beatmaps = createBeatmapDifficulties(archive); - - // remove metadata from difficulties where it matches the set - foreach (BeatmapInfo b in beatmapSet.Beatmaps) - if (beatmapSet.Metadata.Equals(b.Metadata)) - b.Metadata = null; - - // import to beatmap store - import(beatmapSet, context); - - context.SaveChanges(transaction); - return beatmapSet; + undelete(existingHashMatch); + return existingHashMatch; } + + // check if a set already exists with the same online id + if (beatmapSet.OnlineBeatmapSetID != null) + { + var existingOnlineId = beatmaps.BeatmapSets.FirstOrDefault(b => b.OnlineBeatmapSetID == beatmapSet.OnlineBeatmapSetID); + if (existingOnlineId != null) + { + // {Microsoft.EntityFrameworkCore.DbUpdateConcurrencyException: Database operation expected to affect 1 row(s) but actually affected 0 row(s). Data may have been modified or deleted since entities were loaded. See http://go.microsoft.com/fwlink/?LinkId=527962…} + + Delete(existingOnlineId); + beatmaps.Cleanup(s => s.ID == existingOnlineId.ID); + } + } + + beatmapSet.Files = createFileInfos(archive, files); + beatmapSet.Beatmaps = createBeatmapDifficulties(archive); + + // remove metadata from difficulties where it matches the set + foreach (BeatmapInfo b in beatmapSet.Beatmaps) + if (beatmapSet.Metadata.Equals(b.Metadata)) + b.Metadata = null; + + // import to beatmap store + Import(beatmapSet); + return beatmapSet; } } /// /// Import a beatmap from a . /// - /// The beatmap to be imported. - public void Import(BeatmapSetInfo beatmapSetInfo) - { - lock (importContextLock) - { - var context = importContext.Value; - - using (var transaction = context.BeginTransaction()) - { - import(beatmapSetInfo, context); - context.SaveChanges(transaction); - } - } - } + /// The beatmap to be imported. + public void Import(BeatmapSetInfo beatmapSet) => beatmaps.Add(beatmapSet); /// /// Downloads a beatmap. @@ -350,26 +315,22 @@ namespace osu.Game.Beatmaps /// The beatmap set to delete. public void Delete(BeatmapSetInfo beatmapSet) { - lock (importContextLock) + using (var db = contextFactory.GetForWrite()) { - var context = importContext.Value; + var context = db.Context; - using (var transaction = context.BeginTransaction()) + context.ChangeTracker.AutoDetectChangesEnabled = false; + + // re-fetch the beatmap set on the import context. + beatmapSet = context.BeatmapSetInfo.Include(s => s.Files).ThenInclude(f => f.FileInfo).First(s => s.ID == beatmapSet.ID); + + if (beatmaps.Delete(beatmapSet)) { - context.ChangeTracker.AutoDetectChangesEnabled = false; - - // re-fetch the beatmap set on the import context. - beatmapSet = context.BeatmapSetInfo.Include(s => s.Files).ThenInclude(f => f.FileInfo).First(s => s.ID == beatmapSet.ID); - - if (getBeatmapStoreWithContext(context).Delete(beatmapSet)) - { - if (!beatmapSet.Protected) - getFileStoreWithContext(context).Dereference(beatmapSet.Files.Select(f => f.FileInfo).ToArray()); - } - - context.ChangeTracker.AutoDetectChangesEnabled = true; - context.SaveChanges(transaction); + if (!beatmapSet.Protected) + files.Dereference(beatmapSet.Files.Select(f => f.FileInfo).ToArray()); } + + context.ChangeTracker.AutoDetectChangesEnabled = true; } } @@ -417,19 +378,11 @@ namespace osu.Game.Beatmaps if (beatmapSet.Protected) return; - lock (importContextLock) + using (var db = contextFactory.GetForWrite()) { - var context = importContext.Value; - - using (var transaction = context.BeginTransaction()) - { - context.ChangeTracker.AutoDetectChangesEnabled = false; - - undelete(getBeatmapStoreWithContext(context), getFileStoreWithContext(context), beatmapSet); - - context.ChangeTracker.AutoDetectChangesEnabled = true; - context.SaveChanges(transaction); - } + db.Context.ChangeTracker.AutoDetectChangesEnabled = false; + undelete(beatmapSet); + db.Context.ChangeTracker.AutoDetectChangesEnabled = true; } } @@ -452,7 +405,7 @@ namespace osu.Game.Beatmaps /// The store to restore beatmaps from. /// The store to restore beatmap files from. /// The beatmap to restore. - private void undelete(BeatmapStore beatmaps, FileStore files, BeatmapSetInfo beatmapSet) + private void undelete(BeatmapSetInfo beatmapSet) { if (!beatmaps.Undelete(beatmapSet)) return; @@ -578,11 +531,6 @@ namespace osu.Game.Beatmaps notification.State = ProgressNotificationState.Completed; } - /// - /// Import a into the beatmap store. - /// - private void import(BeatmapSetInfo beatmapSet, OsuDbContext context) => getBeatmapStoreWithContext(context).Add(beatmapSet); - /// /// Creates an from a valid storage path. /// @@ -689,19 +637,5 @@ namespace osu.Game.Beatmaps return beatmapInfos; } - - private FileStore getFileStoreWithContext(OsuDbContext context) => new FileStore(() => context, files.Storage); - - private BeatmapStore getBeatmapStoreWithContext(OsuDbContext context) => getBeatmapStoreWithContext(() => context); - - private BeatmapStore getBeatmapStoreWithContext(Func context) - { - var store = new BeatmapStore(context); - store.BeatmapSetAdded += s => BeatmapSetAdded?.Invoke(s); - store.BeatmapSetRemoved += s => BeatmapSetRemoved?.Invoke(s); - store.BeatmapHidden += b => BeatmapHidden?.Invoke(b); - store.BeatmapRestored += b => BeatmapRestored?.Invoke(b); - return store; - } } } diff --git a/osu.Game/Beatmaps/BeatmapStore.cs b/osu.Game/Beatmaps/BeatmapStore.cs index f2c3eddec9..67a2bbbd90 100644 --- a/osu.Game/Beatmaps/BeatmapStore.cs +++ b/osu.Game/Beatmaps/BeatmapStore.cs @@ -20,7 +20,7 @@ namespace osu.Game.Beatmaps public event Action BeatmapHidden; public event Action BeatmapRestored; - public BeatmapStore(Func factory) + public BeatmapStore(DatabaseContextFactory factory) : base(factory) { } @@ -31,24 +31,25 @@ namespace osu.Game.Beatmaps /// The beatmap to add. public void Add(BeatmapSetInfo beatmapSet) { - var context = GetContext(); - - foreach (var beatmap in beatmapSet.Beatmaps.Where(b => b.Metadata != null)) + using (var db = ContextFactory.GetForWrite()) { - // If we detect a new metadata object it'll be attached to the current context so it can be reused - // to prevent duplicate entries when persisting. To accomplish this we look in the cache (.Local) - // of the corresponding table (.Set()) for matching entries to our criteria. - var contextMetadata = context.Set().Local.SingleOrDefault(e => e.Equals(beatmap.Metadata)); - if (contextMetadata != null) - beatmap.Metadata = contextMetadata; - else - context.BeatmapMetadata.Attach(beatmap.Metadata); + var context = db.Context; + + foreach (var beatmap in beatmapSet.Beatmaps.Where(b => b.Metadata != null)) + { + // If we detect a new metadata object it'll be attached to the current context so it can be reused + // to prevent duplicate entries when persisting. To accomplish this we look in the cache (.Local) + // of the corresponding table (.Set()) for matching entries to our criteria. + var contextMetadata = context.Set().Local.SingleOrDefault(e => e.Equals(beatmap.Metadata)); + if (contextMetadata != null) + beatmap.Metadata = contextMetadata; + else + context.BeatmapMetadata.Attach(beatmap.Metadata); + } + + context.BeatmapSetInfo.Attach(beatmapSet); + BeatmapSetAdded?.Invoke(beatmapSet); } - - context.BeatmapSetInfo.Attach(beatmapSet); - context.SaveChanges(); - - BeatmapSetAdded?.Invoke(beatmapSet); } /// @@ -59,10 +60,8 @@ namespace osu.Game.Beatmaps { BeatmapSetRemoved?.Invoke(beatmapSet); - var context = GetContext(); - - context.BeatmapSetInfo.Update(beatmapSet); - context.SaveChanges(); + using (var usage = ContextFactory.GetForWrite()) + usage.Context.BeatmapSetInfo.Update(beatmapSet); BeatmapSetAdded?.Invoke(beatmapSet); } @@ -74,13 +73,13 @@ namespace osu.Game.Beatmaps /// Whether the beatmap's was changed. public bool Delete(BeatmapSetInfo beatmapSet) { - var context = GetContext(); + using ( ContextFactory.GetForWrite()) + { + Refresh(ref beatmapSet, BeatmapSets); - Refresh(ref beatmapSet, BeatmapSets); - - if (beatmapSet.DeletePending) return false; - beatmapSet.DeletePending = true; - context.SaveChanges(); + if (beatmapSet.DeletePending) return false; + beatmapSet.DeletePending = true; + } BeatmapSetRemoved?.Invoke(beatmapSet); return true; @@ -93,13 +92,13 @@ namespace osu.Game.Beatmaps /// Whether the beatmap's was changed. public bool Undelete(BeatmapSetInfo beatmapSet) { - var context = GetContext(); + using ( ContextFactory.GetForWrite()) + { + Refresh(ref beatmapSet, BeatmapSets); - Refresh(ref beatmapSet, BeatmapSets); - - if (!beatmapSet.DeletePending) return false; - beatmapSet.DeletePending = false; - context.SaveChanges(); + if (!beatmapSet.DeletePending) return false; + beatmapSet.DeletePending = false; + } BeatmapSetAdded?.Invoke(beatmapSet); return true; @@ -112,15 +111,16 @@ namespace osu.Game.Beatmaps /// Whether the beatmap's was changed. public bool Hide(BeatmapInfo beatmap) { - var context = GetContext(); + using (ContextFactory.GetForWrite()) + { + Refresh(ref beatmap, Beatmaps); - Refresh(ref beatmap, Beatmaps); + if (beatmap.Hidden) return false; + beatmap.Hidden = true; - if (beatmap.Hidden) return false; - beatmap.Hidden = true; - context.SaveChanges(); + BeatmapHidden?.Invoke(beatmap); + } - BeatmapHidden?.Invoke(beatmap); return true; } @@ -131,13 +131,13 @@ namespace osu.Game.Beatmaps /// Whether the beatmap's was changed. public bool Restore(BeatmapInfo beatmap) { - var context = GetContext(); + using (ContextFactory.GetForWrite()) + { + Refresh(ref beatmap, Beatmaps); - Refresh(ref beatmap, Beatmaps); - - if (!beatmap.Hidden) return false; - beatmap.Hidden = false; - context.SaveChanges(); + if (!beatmap.Hidden) return false; + beatmap.Hidden = false; + } BeatmapRestored?.Invoke(beatmap); return true; @@ -147,34 +147,36 @@ namespace osu.Game.Beatmaps public void Cleanup(Expression> query) { - var context = GetContext(); + using (var usage = ContextFactory.GetForWrite()) + { + var context = usage.Context; - var purgeable = context.BeatmapSetInfo.Where(s => s.DeletePending && !s.Protected) - .Where(query) - .Include(s => s.Beatmaps).ThenInclude(b => b.Metadata) - .Include(s => s.Beatmaps).ThenInclude(b => b.BaseDifficulty) - .Include(s => s.Metadata); + var purgeable = context.BeatmapSetInfo.Where(s => s.DeletePending && !s.Protected) + .Where(query) + .Include(s => s.Beatmaps).ThenInclude(b => b.Metadata) + .Include(s => s.Beatmaps).ThenInclude(b => b.BaseDifficulty) + .Include(s => s.Metadata); - // metadata is M-N so we can't rely on cascades - context.BeatmapMetadata.RemoveRange(purgeable.Select(s => s.Metadata)); - context.BeatmapMetadata.RemoveRange(purgeable.SelectMany(s => s.Beatmaps.Select(b => b.Metadata).Where(m => m != null))); + // metadata is M-N so we can't rely on cascades + context.BeatmapMetadata.RemoveRange(purgeable.Select(s => s.Metadata)); + context.BeatmapMetadata.RemoveRange(purgeable.SelectMany(s => s.Beatmaps.Select(b => b.Metadata).Where(m => m != null))); - // todo: we can probably make cascades work here with a FK in BeatmapDifficulty. just make to make it work correctly. - context.BeatmapDifficulty.RemoveRange(purgeable.SelectMany(s => s.Beatmaps.Select(b => b.BaseDifficulty))); + // todo: we can probably make cascades work here with a FK in BeatmapDifficulty. just make to make it work correctly. + context.BeatmapDifficulty.RemoveRange(purgeable.SelectMany(s => s.Beatmaps.Select(b => b.BaseDifficulty))); - // cascades down to beatmaps. - context.BeatmapSetInfo.RemoveRange(purgeable); - context.SaveChanges(); + // cascades down to beatmaps. + context.BeatmapSetInfo.RemoveRange(purgeable); + } } - public IQueryable BeatmapSets => GetContext().BeatmapSetInfo + public IQueryable BeatmapSets => ContextFactory.Get().BeatmapSetInfo .Include(s => s.Metadata) .Include(s => s.Beatmaps).ThenInclude(s => s.Ruleset) .Include(s => s.Beatmaps).ThenInclude(b => b.BaseDifficulty) .Include(s => s.Beatmaps).ThenInclude(b => b.Metadata) .Include(s => s.Files).ThenInclude(f => f.FileInfo); - public IQueryable Beatmaps => GetContext().BeatmapInfo + public IQueryable Beatmaps => ContextFactory.Get().BeatmapInfo .Include(b => b.BeatmapSet).ThenInclude(s => s.Metadata) .Include(b => b.BeatmapSet).ThenInclude(s => s.Files).ThenInclude(f => f.FileInfo) .Include(b => b.Metadata) diff --git a/osu.Game/Configuration/SettingsStore.cs b/osu.Game/Configuration/SettingsStore.cs index 9b18151c84..7b66002a79 100644 --- a/osu.Game/Configuration/SettingsStore.cs +++ b/osu.Game/Configuration/SettingsStore.cs @@ -12,8 +12,8 @@ namespace osu.Game.Configuration { public event Action SettingChanged; - public SettingsStore(Func createContext) - : base(createContext) + public SettingsStore(DatabaseContextFactory contextFactory) + : base(contextFactory) { } @@ -24,19 +24,16 @@ namespace osu.Game.Configuration /// An optional variant. /// public List Query(int? rulesetId = null, int? variant = null) => - GetContext().DatabasedSetting.Where(b => b.RulesetID == rulesetId && b.Variant == variant).ToList(); + ContextFactory.Get().DatabasedSetting.Where(b => b.RulesetID == rulesetId && b.Variant == variant).ToList(); public void Update(DatabasedSetting setting) { - var context = GetContext(); - - var newValue = setting.Value; - - Refresh(ref setting); - - setting.Value = newValue; - - context.SaveChanges(); + using (ContextFactory.GetForWrite()) + { + var newValue = setting.Value; + Refresh(ref setting); + setting.Value = newValue; + } SettingChanged?.Invoke(); } diff --git a/osu.Game/Database/DatabaseBackedStore.cs b/osu.Game/Database/DatabaseBackedStore.cs index ec9967e097..da66167b14 100644 --- a/osu.Game/Database/DatabaseBackedStore.cs +++ b/osu.Game/Database/DatabaseBackedStore.cs @@ -1,10 +1,8 @@ // Copyright (c) 2007-2018 ppy Pty Ltd . // Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE -using System; using System.Collections.Generic; using System.Linq; -using System.Threading; using Microsoft.EntityFrameworkCore; using osu.Framework.Platform; @@ -17,9 +15,7 @@ namespace osu.Game.Database /// /// Create a new instance (separate from the shared context via for performing isolated operations. /// - protected readonly Func CreateContext; - - private readonly ThreadLocal queryContext; + protected readonly DatabaseContextFactory ContextFactory; /// /// Refresh an instance potentially from a different thread with a local context-tracked instance. @@ -29,33 +25,27 @@ namespace osu.Game.Database /// A valid EF-stored type. protected virtual void Refresh(ref T obj, IEnumerable lookupSource = null) where T : class, IHasPrimaryKey { - var context = GetContext(); - - if (context.Entry(obj).State != EntityState.Detached) return; - - var id = obj.ID; - var foundObject = lookupSource?.SingleOrDefault(t => t.ID == id) ?? context.Find(id); - if (foundObject != null) + using (var usage = ContextFactory.GetForWrite()) { - obj = foundObject; - context.Entry(obj).Reload(); + var context = usage.Context; + + if (context.Entry(obj).State != EntityState.Detached) return; + + var id = obj.ID; + var foundObject = lookupSource?.SingleOrDefault(t => t.ID == id) ?? context.Find(id); + if (foundObject != null) + { + obj = foundObject; + context.Entry(obj).Reload(); + } + else + context.Add(obj); } - else - context.Add(obj); } - /// - /// Retrieve a shared context for performing lookups (or write operations on the update thread, for now). - /// - protected OsuDbContext GetContext() => queryContext.Value; - - protected DatabaseBackedStore(Func createContext, Storage storage = null) + protected DatabaseBackedStore(DatabaseContextFactory contextFactory, Storage storage = null) { - CreateContext = createContext; - - // todo: while this seems to work quite well, we need to consider that contexts could enter a state where they are never cleaned up. - queryContext = new ThreadLocal(CreateContext); - + ContextFactory = contextFactory; Storage = storage; } diff --git a/osu.Game/Database/DatabaseContextFactory.cs b/osu.Game/Database/DatabaseContextFactory.cs index b1917d92c4..c092ed377f 100644 --- a/osu.Game/Database/DatabaseContextFactory.cs +++ b/osu.Game/Database/DatabaseContextFactory.cs @@ -1,6 +1,7 @@ // Copyright (c) 2007-2018 ppy Pty Ltd . // Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE +using System.Threading; using osu.Framework.Platform; namespace osu.Game.Database @@ -11,17 +12,70 @@ namespace osu.Game.Database private const string database_name = @"client"; + private ThreadLocal threadContexts; + + private readonly object writeLock = new object(); + + private OsuDbContext writeContext; + + private volatile int currentWriteUsages; + public DatabaseContextFactory(GameHost host) { this.host = host; + recycleThreadContexts(); } - public OsuDbContext GetContext() => new OsuDbContext(host.Storage.GetDatabaseConnectionString(database_name)); + /// + /// Get a context for read-only usage. + /// + public OsuDbContext Get() => threadContexts.Value; + + /// + /// Request a context for write usage. Can be consumed in a nested fashion (and will return the same underlying context). + /// This method may block if a write is already active on a different thread. + /// + /// A usage containing a usable context. + public DatabaseWriteUsage GetForWrite() + { + lock (writeLock) + { + var usage = new DatabaseWriteUsage(writeContext ?? (writeContext = threadContexts.Value), usageCompleted); + Interlocked.Increment(ref currentWriteUsages); + return usage; + } + } + + private void usageCompleted(DatabaseWriteUsage usage) + { + int usages = Interlocked.Decrement(ref currentWriteUsages); + if (usages == 0) + { + writeContext.Dispose(); + writeContext = null; + + // once all writes are complete, we want to refresh thread-specific contexts to make sure they don't have stale local caches. + recycleThreadContexts(); + } + } + + private void recycleThreadContexts() => threadContexts = new ThreadLocal(CreateContext); + + protected virtual OsuDbContext CreateContext() + { + var ctx = new OsuDbContext(host.Storage.GetDatabaseConnectionString(database_name)); + ctx.Database.AutoTransactionsEnabled = false; + + return ctx; + } public void ResetDatabase() { - // todo: we probably want to make sure there are no active contexts before performing this operation. - host.Storage.DeleteDatabase(database_name); + lock (writeLock) + { + recycleThreadContexts(); + host.Storage.DeleteDatabase(database_name); + } } } } diff --git a/osu.Game/Database/DatabaseWriteUsage.cs b/osu.Game/Database/DatabaseWriteUsage.cs new file mode 100644 index 0000000000..0dc5a4cfe9 --- /dev/null +++ b/osu.Game/Database/DatabaseWriteUsage.cs @@ -0,0 +1,28 @@ +// Copyright (c) 2007-2018 ppy Pty Ltd . +// Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE + +using System; +using Microsoft.EntityFrameworkCore.Storage; + +namespace osu.Game.Database +{ + public class DatabaseWriteUsage : IDisposable + { + public readonly OsuDbContext Context; + private readonly IDbContextTransaction transaction; + private readonly Action usageCompleted; + + public DatabaseWriteUsage(OsuDbContext context, Action onCompleted) + { + Context = context; + transaction = Context.BeginTransaction(); + usageCompleted = onCompleted; + } + + public void Dispose() + { + Context.SaveChanges(transaction); + usageCompleted?.Invoke(this); + } + } +} diff --git a/osu.Game/Database/SingletonContextFactory.cs b/osu.Game/Database/SingletonContextFactory.cs new file mode 100644 index 0000000000..88a43dc836 --- /dev/null +++ b/osu.Game/Database/SingletonContextFactory.cs @@ -0,0 +1,21 @@ +// Copyright (c) 2007-2018 ppy Pty Ltd . +// Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE + +namespace osu.Game.Database +{ + public class SingletonContextFactory : DatabaseContextFactory + { + private readonly OsuDbContext context; + + public SingletonContextFactory(OsuDbContext context) + : base(null) + { + this.context = context; + } + + protected override OsuDbContext CreateContext() + { + return context; + } + } +} diff --git a/osu.Game/IO/FileStore.cs b/osu.Game/IO/FileStore.cs index 31c608a5f4..1bfe4db81a 100644 --- a/osu.Game/IO/FileStore.cs +++ b/osu.Game/IO/FileStore.cs @@ -21,86 +21,91 @@ namespace osu.Game.IO public new Storage Storage => base.Storage; - public FileStore(Func createContext, Storage storage) : base(createContext, storage.GetStorageForDirectory(@"files")) + public FileStore(DatabaseContextFactory contextFactory, Storage storage) : base(contextFactory, storage.GetStorageForDirectory(@"files")) { Store = new StorageBackedResourceStore(Storage); } public FileInfo Add(Stream data, bool reference = true) { - var context = GetContext(); - - string hash = data.ComputeSHA2Hash(); - - var existing = context.FileInfo.FirstOrDefault(f => f.Hash == hash); - - var info = existing ?? new FileInfo { Hash = hash }; - - string path = info.StoragePath; - - // we may be re-adding a file to fix missing store entries. - if (!Storage.Exists(path)) + using (var usage = ContextFactory.GetForWrite()) { - data.Seek(0, SeekOrigin.Begin); + var context = usage.Context; - using (var output = Storage.GetStream(path, FileAccess.Write)) - data.CopyTo(output); + string hash = data.ComputeSHA2Hash(); - data.Seek(0, SeekOrigin.Begin); + var existing = context.FileInfo.FirstOrDefault(f => f.Hash == hash); + + var info = existing ?? new FileInfo { Hash = hash }; + + string path = info.StoragePath; + + // we may be re-adding a file to fix missing store entries. + if (!Storage.Exists(path)) + { + data.Seek(0, SeekOrigin.Begin); + + using (var output = Storage.GetStream(path, FileAccess.Write)) + data.CopyTo(output); + + data.Seek(0, SeekOrigin.Begin); + } + + if (reference || existing == null) + Reference(info); + + return info; } - - if (reference || existing == null) - Reference(info); - - return info; } - public void Reference(params FileInfo[] files) => reference(GetContext(), files); - - private void reference(OsuDbContext context, FileInfo[] files) + public void Reference(params FileInfo[] files) { - foreach (var f in files.GroupBy(f => f.ID)) + using (var usage = ContextFactory.GetForWrite()) { - var refetch = context.Find(f.First().ID) ?? f.First(); - refetch.ReferenceCount += f.Count(); - context.FileInfo.Update(refetch); - } + var context = usage.Context; - context.SaveChanges(); + foreach (var f in files.GroupBy(f => f.ID)) + { + var refetch = context.Find(f.First().ID) ?? f.First(); + refetch.ReferenceCount += f.Count(); + context.FileInfo.Update(refetch); + } + } } - public void Dereference(params FileInfo[] files) => dereference(GetContext(), files); - - private void dereference(OsuDbContext context, FileInfo[] files) + public void Dereference(params FileInfo[] files) { - foreach (var f in files.GroupBy(f => f.ID)) + using (var usage = ContextFactory.GetForWrite()) { - var refetch = context.FileInfo.Find(f.Key); - refetch.ReferenceCount -= f.Count(); - context.FileInfo.Update(refetch); + var context = usage.Context; + foreach (var f in files.GroupBy(f => f.ID)) + { + var refetch = context.FileInfo.Find(f.Key); + refetch.ReferenceCount -= f.Count(); + context.FileInfo.Update(refetch); + } } - - context.SaveChanges(); } public override void Cleanup() { - var context = GetContext(); - - foreach (var f in context.FileInfo.Where(f => f.ReferenceCount < 1)) + using (var usage = ContextFactory.GetForWrite()) { - try + var context = usage.Context; + + foreach (var f in context.FileInfo.Where(f => f.ReferenceCount < 1)) { - Storage.Delete(f.StoragePath); - context.FileInfo.Remove(f); - } - catch (Exception e) - { - Logger.Error(e, $@"Could not delete beatmap {f}"); + try + { + Storage.Delete(f.StoragePath); + context.FileInfo.Remove(f); + } + catch (Exception e) + { + Logger.Error(e, $@"Could not delete beatmap {f}"); + } } } - - context.SaveChanges(); } } } diff --git a/osu.Game/Input/KeyBindingStore.cs b/osu.Game/Input/KeyBindingStore.cs index 92159ab491..4aad684959 100644 --- a/osu.Game/Input/KeyBindingStore.cs +++ b/osu.Game/Input/KeyBindingStore.cs @@ -16,14 +16,17 @@ namespace osu.Game.Input { public event Action KeyBindingChanged; - public KeyBindingStore(Func createContext, RulesetStore rulesets, Storage storage = null) - : base(createContext, storage) + public KeyBindingStore(DatabaseContextFactory contextFactory, RulesetStore rulesets, Storage storage = null) + : base(contextFactory, storage) { - foreach (var info in rulesets.AvailableRulesets) + using (ContextFactory.GetForWrite()) { - var ruleset = info.CreateInstance(); - foreach (var variant in ruleset.AvailableVariants) - insertDefaults(ruleset.GetDefaultKeyBindings(variant), info.ID, variant); + foreach (var info in rulesets.AvailableRulesets) + { + var ruleset = info.CreateInstance(); + foreach (var variant in ruleset.AvailableVariants) + insertDefaults(ruleset.GetDefaultKeyBindings(variant), info.ID, variant); + } } } @@ -31,10 +34,10 @@ namespace osu.Game.Input private void insertDefaults(IEnumerable defaults, int? rulesetId = null, int? variant = null) { - var context = GetContext(); - - using (var transaction = context.BeginTransaction()) + using (var usage = ContextFactory.GetForWrite()) { + var context = usage.Context; + // compare counts in database vs defaults foreach (var group in defaults.GroupBy(k => k.Action)) { @@ -54,8 +57,6 @@ namespace osu.Game.Input Variant = variant }); } - - context.SaveChanges(transaction); } } @@ -66,19 +67,16 @@ namespace osu.Game.Input /// An optional variant. /// public List Query(int? rulesetId = null, int? variant = null) => - GetContext().DatabasedKeyBinding.Where(b => b.RulesetID == rulesetId && b.Variant == variant).ToList(); + ContextFactory.Get().DatabasedKeyBinding.Where(b => b.RulesetID == rulesetId && b.Variant == variant).ToList(); public void Update(KeyBinding keyBinding) { - var dbKeyBinding = (DatabasedKeyBinding)keyBinding; - - var context = GetContext(); - - Refresh(ref dbKeyBinding); - - dbKeyBinding.KeyCombination = keyBinding.KeyCombination; - - context.SaveChanges(); + using (ContextFactory.GetForWrite()) + { + var dbKeyBinding = (DatabasedKeyBinding)keyBinding; + Refresh(ref dbKeyBinding); + dbKeyBinding.KeyCombination = keyBinding.KeyCombination; + } KeyBindingChanged?.Invoke(); } diff --git a/osu.Game/OsuGameBase.cs b/osu.Game/OsuGameBase.cs index a7eac27056..505577416d 100644 --- a/osu.Game/OsuGameBase.cs +++ b/osu.Game/OsuGameBase.cs @@ -106,12 +106,12 @@ namespace osu.Game Token = LocalConfig.Get(OsuSetting.Token) }); - dependencies.Cache(RulesetStore = new RulesetStore(contextFactory.GetContext)); - dependencies.Cache(FileStore = new FileStore(contextFactory.GetContext, Host.Storage)); - dependencies.Cache(BeatmapManager = new BeatmapManager(Host.Storage, contextFactory.GetContext, RulesetStore, API, Host)); - dependencies.Cache(ScoreStore = new ScoreStore(Host.Storage, contextFactory.GetContext, Host, BeatmapManager, RulesetStore)); - dependencies.Cache(KeyBindingStore = new KeyBindingStore(contextFactory.GetContext, RulesetStore)); - dependencies.Cache(SettingsStore = new SettingsStore(contextFactory.GetContext)); + dependencies.Cache(RulesetStore = new RulesetStore(contextFactory)); + dependencies.Cache(FileStore = new FileStore(contextFactory, Host.Storage)); + dependencies.Cache(BeatmapManager = new BeatmapManager(Host.Storage, contextFactory, RulesetStore, API, Host)); + dependencies.Cache(ScoreStore = new ScoreStore(Host.Storage, contextFactory, Host, BeatmapManager, RulesetStore)); + dependencies.Cache(KeyBindingStore = new KeyBindingStore(contextFactory, RulesetStore)); + dependencies.Cache(SettingsStore = new SettingsStore(contextFactory)); dependencies.Cache(new OsuColour()); //this completely overrides the framework default. will need to change once we make a proper FontStore. @@ -179,8 +179,8 @@ namespace osu.Game { try { - using (var context = contextFactory.GetContext()) - context.Migrate(); + using (var db = contextFactory.GetForWrite()) + db.Context.Migrate(); } catch (MigrationFailedException e) { @@ -191,8 +191,8 @@ namespace osu.Game contextFactory.ResetDatabase(); Logger.Log("Database purged successfully.", LoggingTarget.Database, LogLevel.Important); - using (var context = contextFactory.GetContext()) - context.Migrate(); + using (var db = contextFactory.GetForWrite()) + db.Context.Migrate(); } } diff --git a/osu.Game/Rulesets/RulesetStore.cs b/osu.Game/Rulesets/RulesetStore.cs index 01e3b6848f..f66a126211 100644 --- a/osu.Game/Rulesets/RulesetStore.cs +++ b/osu.Game/Rulesets/RulesetStore.cs @@ -25,7 +25,7 @@ namespace osu.Game.Rulesets loadRulesetFromFile(file); } - public RulesetStore(Func factory) + public RulesetStore(DatabaseContextFactory factory) : base(factory) { AddMissingRulesets(); @@ -56,47 +56,50 @@ namespace osu.Game.Rulesets protected void AddMissingRulesets() { - var context = GetContext(); - - var instances = loaded_assemblies.Values.Select(r => (Ruleset)Activator.CreateInstance(r, (RulesetInfo)null)).ToList(); - - //add all legacy modes in correct order - foreach (var r in instances.Where(r => r.LegacyID >= 0).OrderBy(r => r.LegacyID)) + using (var usage = ContextFactory.GetForWrite()) { - if (context.RulesetInfo.SingleOrDefault(rsi => rsi.ID == r.RulesetInfo.ID) == null) - context.RulesetInfo.Add(r.RulesetInfo); - } + var context = usage.Context; - context.SaveChanges(); + var instances = loaded_assemblies.Values.Select(r => (Ruleset)Activator.CreateInstance(r, (RulesetInfo)null)).ToList(); - //add any other modes - foreach (var r in instances.Where(r => r.LegacyID < 0)) - if (context.RulesetInfo.FirstOrDefault(ri => ri.InstantiationInfo == r.RulesetInfo.InstantiationInfo) == null) - context.RulesetInfo.Add(r.RulesetInfo); - - context.SaveChanges(); - - //perform a consistency check - foreach (var r in context.RulesetInfo) - { - try + //add all legacy modes in correct order + foreach (var r in instances.Where(r => r.LegacyID >= 0).OrderBy(r => r.LegacyID)) { - var instance = r.CreateInstance(); - - r.Name = instance.Description; - r.ShortName = instance.ShortName; - - r.Available = true; + if (context.RulesetInfo.SingleOrDefault(rsi => rsi.ID == r.RulesetInfo.ID) == null) + context.RulesetInfo.Add(r.RulesetInfo); } - catch + + context.SaveChanges(); + + //add any other modes + foreach (var r in instances.Where(r => r.LegacyID < 0)) + if (context.RulesetInfo.FirstOrDefault(ri => ri.InstantiationInfo == r.RulesetInfo.InstantiationInfo) == null) + context.RulesetInfo.Add(r.RulesetInfo); + + context.SaveChanges(); + + //perform a consistency check + foreach (var r in context.RulesetInfo) { - r.Available = false; + try + { + var instance = r.CreateInstance(); + + r.Name = instance.Description; + r.ShortName = instance.ShortName; + + r.Available = true; + } + catch + { + r.Available = false; + } } + + context.SaveChanges(); + + AvailableRulesets = context.RulesetInfo.Where(r => r.Available).ToList(); } - - context.SaveChanges(); - - AvailableRulesets = context.RulesetInfo.Where(r => r.Available).ToList(); } private static void loadRulesetFromFile(string file) diff --git a/osu.Game/Rulesets/Scoring/ScoreStore.cs b/osu.Game/Rulesets/Scoring/ScoreStore.cs index d21ca79736..8bde2747a2 100644 --- a/osu.Game/Rulesets/Scoring/ScoreStore.cs +++ b/osu.Game/Rulesets/Scoring/ScoreStore.cs @@ -1,7 +1,6 @@ // Copyright (c) 2007-2018 ppy Pty Ltd . // Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE -using System; using System.Collections.Generic; using System.IO; using osu.Framework.Platform; @@ -27,7 +26,7 @@ namespace osu.Game.Rulesets.Scoring // ReSharper disable once NotAccessedField.Local (we should keep a reference to this so it is not finalised) private ScoreIPCChannel ipc; - public ScoreStore(Storage storage, Func factory, IIpcHost importHost = null, BeatmapManager beatmaps = null, RulesetStore rulesets = null) : base(factory) + public ScoreStore(Storage storage, DatabaseContextFactory factory, IIpcHost importHost = null, BeatmapManager beatmaps = null, RulesetStore rulesets = null) : base(factory) { this.storage = storage; this.beatmaps = beatmaps; diff --git a/osu.Game/osu.Game.csproj b/osu.Game/osu.Game.csproj index c16767c02c..71f1629c19 100644 --- a/osu.Game/osu.Game.csproj +++ b/osu.Game/osu.Game.csproj @@ -275,7 +275,9 @@ + + From 8b37fde15b51668f5bbe4e4c3b2d6cbaae2fb459 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 12 Feb 2018 19:57:21 +0900 Subject: [PATCH 24/33] Only write when writes occur Also add finaliser logic for safety. Also better threading. Also more cleanup. --- osu.Game/Beatmaps/BeatmapManager.cs | 36 +++++++------------ osu.Game/Beatmaps/BeatmapStore.cs | 17 ++++++--- osu.Game/Database/DatabaseBackedStore.cs | 3 -- osu.Game/Database/DatabaseContextFactory.cs | 39 +++++++++++++++------ osu.Game/Database/DatabaseWriteUsage.cs | 22 ++++++++++-- osu.Game/Database/OsuDbContext.cs | 2 +- osu.Game/IO/FileStore.cs | 9 +++-- osu.Game/Input/KeyBindingStore.cs | 8 +++-- 8 files changed, 86 insertions(+), 50 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index 4ec153c78f..41ea293938 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -172,7 +172,7 @@ namespace osu.Game.Beatmaps /// The beatmap to be imported. public BeatmapSetInfo Import(ArchiveReader archive) { - using ( contextFactory.GetForWrite()) // used to share a context for full import. keep in mind this will block all writes. + using (contextFactory.GetForWrite()) // used to share a context for full import. keep in mind this will block all writes. { // create a new set info (don't yet add to database) var beatmapSet = createBeatmapSetInfo(archive); @@ -181,7 +181,7 @@ namespace osu.Game.Beatmaps var existingHashMatch = beatmaps.BeatmapSets.FirstOrDefault(b => b.Hash == beatmapSet.Hash); if (existingHashMatch != null) { - undelete(existingHashMatch); + Undelete(existingHashMatch); return existingHashMatch; } @@ -315,9 +315,9 @@ namespace osu.Game.Beatmaps /// The beatmap set to delete. public void Delete(BeatmapSetInfo beatmapSet) { - using (var db = contextFactory.GetForWrite()) + using (var usage = contextFactory.GetForWrite()) { - var context = db.Context; + var context = usage.Context; context.ChangeTracker.AutoDetectChangesEnabled = false; @@ -378,11 +378,16 @@ namespace osu.Game.Beatmaps if (beatmapSet.Protected) return; - using (var db = contextFactory.GetForWrite()) + using (var usage = contextFactory.GetForWrite()) { - db.Context.ChangeTracker.AutoDetectChangesEnabled = false; - undelete(beatmapSet); - db.Context.ChangeTracker.AutoDetectChangesEnabled = true; + usage.Context.ChangeTracker.AutoDetectChangesEnabled = false; + + if (!beatmaps.Undelete(beatmapSet)) return; + + if (!beatmapSet.Protected) + files.Reference(beatmapSet.Files.Select(f => f.FileInfo).ToArray()); + + usage.Context.ChangeTracker.AutoDetectChangesEnabled = true; } } @@ -398,21 +403,6 @@ namespace osu.Game.Beatmaps /// The beatmap difficulty to restore. public void Restore(BeatmapInfo beatmap) => beatmaps.Restore(beatmap); - /// - /// Returns a to a usable state if it has previously been deleted but not yet purged. - /// Is a no-op for already usable beatmaps. - /// - /// The store to restore beatmaps from. - /// The store to restore beatmap files from. - /// The beatmap to restore. - private void undelete(BeatmapSetInfo beatmapSet) - { - if (!beatmaps.Undelete(beatmapSet)) return; - - if (!beatmapSet.Protected) - files.Reference(beatmapSet.Files.Select(f => f.FileInfo).ToArray()); - } - /// /// Retrieve a instance for the provided /// diff --git a/osu.Game/Beatmaps/BeatmapStore.cs b/osu.Game/Beatmaps/BeatmapStore.cs index 67a2bbbd90..7a1dc763f0 100644 --- a/osu.Game/Beatmaps/BeatmapStore.cs +++ b/osu.Game/Beatmaps/BeatmapStore.cs @@ -31,9 +31,9 @@ namespace osu.Game.Beatmaps /// The beatmap to add. public void Add(BeatmapSetInfo beatmapSet) { - using (var db = ContextFactory.GetForWrite()) + using (var usage = ContextFactory.GetForWrite()) { - var context = db.Context; + var context = usage.Context; foreach (var beatmap in beatmapSet.Beatmaps.Where(b => b.Metadata != null)) { @@ -48,6 +48,7 @@ namespace osu.Game.Beatmaps } context.BeatmapSetInfo.Attach(beatmapSet); + BeatmapSetAdded?.Invoke(beatmapSet); } } @@ -73,11 +74,12 @@ namespace osu.Game.Beatmaps /// Whether the beatmap's was changed. public bool Delete(BeatmapSetInfo beatmapSet) { - using ( ContextFactory.GetForWrite()) + using (ContextFactory.GetForWrite()) { Refresh(ref beatmapSet, BeatmapSets); if (beatmapSet.DeletePending) return false; + beatmapSet.DeletePending = true; } @@ -92,11 +94,12 @@ namespace osu.Game.Beatmaps /// Whether the beatmap's was changed. public bool Undelete(BeatmapSetInfo beatmapSet) { - using ( ContextFactory.GetForWrite()) + using (ContextFactory.GetForWrite()) { Refresh(ref beatmapSet, BeatmapSets); if (!beatmapSet.DeletePending) return false; + beatmapSet.DeletePending = false; } @@ -116,6 +119,7 @@ namespace osu.Game.Beatmaps Refresh(ref beatmap, Beatmaps); if (beatmap.Hidden) return false; + beatmap.Hidden = true; BeatmapHidden?.Invoke(beatmap); @@ -136,6 +140,7 @@ namespace osu.Game.Beatmaps Refresh(ref beatmap, Beatmaps); if (!beatmap.Hidden) return false; + beatmap.Hidden = false; } @@ -155,7 +160,9 @@ namespace osu.Game.Beatmaps .Where(query) .Include(s => s.Beatmaps).ThenInclude(b => b.Metadata) .Include(s => s.Beatmaps).ThenInclude(b => b.BaseDifficulty) - .Include(s => s.Metadata); + .Include(s => s.Metadata).ToList(); + + if (!purgeable.Any()) return; // metadata is M-N so we can't rely on cascades context.BeatmapMetadata.RemoveRange(purgeable.Select(s => s.Metadata)); diff --git a/osu.Game/Database/DatabaseBackedStore.cs b/osu.Game/Database/DatabaseBackedStore.cs index da66167b14..0b2f34f6d1 100644 --- a/osu.Game/Database/DatabaseBackedStore.cs +++ b/osu.Game/Database/DatabaseBackedStore.cs @@ -34,10 +34,7 @@ namespace osu.Game.Database var id = obj.ID; var foundObject = lookupSource?.SingleOrDefault(t => t.ID == id) ?? context.Find(id); if (foundObject != null) - { obj = foundObject; - context.Entry(obj).Reload(); - } else context.Add(obj); } diff --git a/osu.Game/Database/DatabaseContextFactory.cs b/osu.Game/Database/DatabaseContextFactory.cs index c092ed377f..2291374e46 100644 --- a/osu.Game/Database/DatabaseContextFactory.cs +++ b/osu.Game/Database/DatabaseContextFactory.cs @@ -1,6 +1,7 @@ // Copyright (c) 2007-2018 ppy Pty Ltd . // Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE +using System.Diagnostics; using System.Threading; using osu.Framework.Platform; @@ -18,6 +19,7 @@ namespace osu.Game.Database private OsuDbContext writeContext; + private bool currentWriteDidWrite; private volatile int currentWriteUsages; public DatabaseContextFactory(GameHost host) @@ -38,24 +40,41 @@ namespace osu.Game.Database /// A usage containing a usable context. public DatabaseWriteUsage GetForWrite() { - lock (writeLock) - { - var usage = new DatabaseWriteUsage(writeContext ?? (writeContext = threadContexts.Value), usageCompleted); - Interlocked.Increment(ref currentWriteUsages); - return usage; - } + Monitor.Enter(writeLock); + + Trace.Assert(currentWriteUsages == 0, "Database writes in a bad state"); + Interlocked.Increment(ref currentWriteUsages); + + return new DatabaseWriteUsage(writeContext ?? (writeContext = threadContexts.Value), usageCompleted); } private void usageCompleted(DatabaseWriteUsage usage) { int usages = Interlocked.Decrement(ref currentWriteUsages); - if (usages == 0) + + try { - writeContext.Dispose(); + currentWriteDidWrite |= usage.PerformedWrite; + + if (usages > 0) return; + + + if (currentWriteDidWrite) + { + writeContext.Dispose(); + currentWriteDidWrite = false; + + // once all writes are complete, we want to refresh thread-specific contexts to make sure they don't have stale local caches. + recycleThreadContexts(); + } + + // always set to null (even when a write didn't occur) so we get the correct thread context on next write request. writeContext = null; - // once all writes are complete, we want to refresh thread-specific contexts to make sure they don't have stale local caches. - recycleThreadContexts(); + } + finally + { + Monitor.Exit(writeLock); } } diff --git a/osu.Game/Database/DatabaseWriteUsage.cs b/osu.Game/Database/DatabaseWriteUsage.cs index 0dc5a4cfe9..52dd0ee268 100644 --- a/osu.Game/Database/DatabaseWriteUsage.cs +++ b/osu.Game/Database/DatabaseWriteUsage.cs @@ -19,10 +19,28 @@ namespace osu.Game.Database usageCompleted = onCompleted; } + public bool PerformedWrite { get; private set; } + + private bool isDisposed; + + protected void Dispose(bool disposing) + { + if (isDisposed) return; + isDisposed = true; + + PerformedWrite |= Context.SaveChanges(transaction) > 0; + usageCompleted?.Invoke(this); + } + public void Dispose() { - Context.SaveChanges(transaction); - usageCompleted?.Invoke(this); + Dispose(true); + GC.SuppressFinalize(this); + } + + ~DatabaseWriteUsage() + { + Dispose(false); } } } diff --git a/osu.Game/Database/OsuDbContext.cs b/osu.Game/Database/OsuDbContext.cs index cf29ae4496..e83b30595e 100644 --- a/osu.Game/Database/OsuDbContext.cs +++ b/osu.Game/Database/OsuDbContext.cs @@ -111,7 +111,7 @@ namespace osu.Game.Database public int SaveChanges(IDbContextTransaction transaction = null) { var ret = base.SaveChanges(); - transaction?.Commit(); + if (ret > 0) transaction?.Commit(); return ret; } diff --git a/osu.Game/IO/FileStore.cs b/osu.Game/IO/FileStore.cs index 1bfe4db81a..9889088dc4 100644 --- a/osu.Game/IO/FileStore.cs +++ b/osu.Game/IO/FileStore.cs @@ -30,11 +30,9 @@ namespace osu.Game.IO { using (var usage = ContextFactory.GetForWrite()) { - var context = usage.Context; - string hash = data.ComputeSHA2Hash(); - var existing = context.FileInfo.FirstOrDefault(f => f.Hash == hash); + var existing = usage.Context.FileInfo.FirstOrDefault(f => f.Hash == hash); var info = existing ?? new FileInfo { Hash = hash }; @@ -60,6 +58,8 @@ namespace osu.Game.IO public void Reference(params FileInfo[] files) { + if (files.Length == 0) return; + using (var usage = ContextFactory.GetForWrite()) { var context = usage.Context; @@ -75,9 +75,12 @@ namespace osu.Game.IO public void Dereference(params FileInfo[] files) { + if (files.Length == 0) return; + using (var usage = ContextFactory.GetForWrite()) { var context = usage.Context; + foreach (var f in files.GroupBy(f => f.ID)) { var refetch = context.FileInfo.Find(f.Key); diff --git a/osu.Game/Input/KeyBindingStore.cs b/osu.Game/Input/KeyBindingStore.cs index 4aad684959..33cb0911a8 100644 --- a/osu.Game/Input/KeyBindingStore.cs +++ b/osu.Game/Input/KeyBindingStore.cs @@ -36,8 +36,6 @@ namespace osu.Game.Input { using (var usage = ContextFactory.GetForWrite()) { - var context = usage.Context; - // compare counts in database vs defaults foreach (var group in defaults.GroupBy(k => k.Action)) { @@ -49,7 +47,7 @@ namespace osu.Game.Input foreach (var insertable in group.Skip(count).Take(aimCount - count)) // insert any defaults which are missing. - context.DatabasedKeyBinding.Add(new DatabasedKeyBinding + usage.Context.DatabasedKeyBinding.Add(new DatabasedKeyBinding { KeyCombination = insertable.KeyCombination, Action = insertable.Action, @@ -75,6 +73,10 @@ namespace osu.Game.Input { var dbKeyBinding = (DatabasedKeyBinding)keyBinding; Refresh(ref dbKeyBinding); + + if (dbKeyBinding.KeyCombination.Equals(keyBinding.KeyCombination)) + return; + dbKeyBinding.KeyCombination = keyBinding.KeyCombination; } From 64cda9fd0f6ae85ea42838ff4144b41eb1ec2b56 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 12 Feb 2018 22:16:09 +0900 Subject: [PATCH 25/33] Remove incorrect assert assumption --- osu.Game/Database/DatabaseContextFactory.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/osu.Game/Database/DatabaseContextFactory.cs b/osu.Game/Database/DatabaseContextFactory.cs index 2291374e46..eaeea0b35e 100644 --- a/osu.Game/Database/DatabaseContextFactory.cs +++ b/osu.Game/Database/DatabaseContextFactory.cs @@ -1,7 +1,6 @@ // Copyright (c) 2007-2018 ppy Pty Ltd . // Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE -using System.Diagnostics; using System.Threading; using osu.Framework.Platform; @@ -42,7 +41,6 @@ namespace osu.Game.Database { Monitor.Enter(writeLock); - Trace.Assert(currentWriteUsages == 0, "Database writes in a bad state"); Interlocked.Increment(ref currentWriteUsages); return new DatabaseWriteUsage(writeContext ?? (writeContext = threadContexts.Value), usageCompleted); From a738664167a579f726828ea9b0cc445b0b1a939d Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Mon, 12 Feb 2018 23:10:05 +0900 Subject: [PATCH 26/33] Add interface for database context factory --- .../Visual/TestCasePlaySongSelect.cs | 2 +- osu.Game/Beatmaps/BeatmapManager.cs | 4 ++-- osu.Game/Beatmaps/BeatmapStore.cs | 2 +- osu.Game/Database/DatabaseBackedStore.cs | 4 ++-- osu.Game/Database/DatabaseContextFactory.cs | 2 +- osu.Game/Database/IDatabaseContextFactory.cs | 20 +++++++++++++++++++ osu.Game/Database/SingletonContextFactory.cs | 10 ++++------ osu.Game/IO/FileStore.cs | 2 +- osu.Game/Rulesets/RulesetStore.cs | 2 +- osu.Game/osu.Game.csproj | 1 + 10 files changed, 34 insertions(+), 15 deletions(-) create mode 100644 osu.Game/Database/IDatabaseContextFactory.cs diff --git a/osu.Game.Tests/Visual/TestCasePlaySongSelect.cs b/osu.Game.Tests/Visual/TestCasePlaySongSelect.cs index f54eb77c6b..8bb0d152f6 100644 --- a/osu.Game.Tests/Visual/TestCasePlaySongSelect.cs +++ b/osu.Game.Tests/Visual/TestCasePlaySongSelect.cs @@ -63,7 +63,7 @@ namespace osu.Game.Tests.Visual var storage = new TestStorage(@"TestCasePlaySongSelect"); // this is by no means clean. should be replacing inside of OsuGameBase somehow. - DatabaseContextFactory factory = new SingletonContextFactory(new OsuDbContext()); + IDatabaseContextFactory factory = new SingletonContextFactory(new OsuDbContext()); dependencies.Cache(rulesets = new RulesetStore(factory)); dependencies.Cache(manager = new BeatmapManager(storage, factory, rulesets, null) diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index 41ea293938..5748062fd5 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -60,7 +60,7 @@ namespace osu.Game.Beatmaps /// public WorkingBeatmap DefaultBeatmap { private get; set; } - private readonly DatabaseContextFactory contextFactory; + private readonly IDatabaseContextFactory contextFactory; private readonly FileStore files; @@ -85,7 +85,7 @@ namespace osu.Game.Beatmaps /// public Func GetStableStorage { private get; set; } - public BeatmapManager(Storage storage, DatabaseContextFactory contextFactory, RulesetStore rulesets, APIAccess api, IIpcHost importHost = null) + public BeatmapManager(Storage storage, IDatabaseContextFactory contextFactory, RulesetStore rulesets, APIAccess api, IIpcHost importHost = null) { this.contextFactory = contextFactory; diff --git a/osu.Game/Beatmaps/BeatmapStore.cs b/osu.Game/Beatmaps/BeatmapStore.cs index 7a1dc763f0..29373c0715 100644 --- a/osu.Game/Beatmaps/BeatmapStore.cs +++ b/osu.Game/Beatmaps/BeatmapStore.cs @@ -20,7 +20,7 @@ namespace osu.Game.Beatmaps public event Action BeatmapHidden; public event Action BeatmapRestored; - public BeatmapStore(DatabaseContextFactory factory) + public BeatmapStore(IDatabaseContextFactory factory) : base(factory) { } diff --git a/osu.Game/Database/DatabaseBackedStore.cs b/osu.Game/Database/DatabaseBackedStore.cs index 0b2f34f6d1..cf46b66422 100644 --- a/osu.Game/Database/DatabaseBackedStore.cs +++ b/osu.Game/Database/DatabaseBackedStore.cs @@ -15,7 +15,7 @@ namespace osu.Game.Database /// /// Create a new instance (separate from the shared context via for performing isolated operations. /// - protected readonly DatabaseContextFactory ContextFactory; + protected readonly IDatabaseContextFactory ContextFactory; /// /// Refresh an instance potentially from a different thread with a local context-tracked instance. @@ -40,7 +40,7 @@ namespace osu.Game.Database } } - protected DatabaseBackedStore(DatabaseContextFactory contextFactory, Storage storage = null) + protected DatabaseBackedStore(IDatabaseContextFactory contextFactory, Storage storage = null) { ContextFactory = contextFactory; Storage = storage; diff --git a/osu.Game/Database/DatabaseContextFactory.cs b/osu.Game/Database/DatabaseContextFactory.cs index eaeea0b35e..002e9e456d 100644 --- a/osu.Game/Database/DatabaseContextFactory.cs +++ b/osu.Game/Database/DatabaseContextFactory.cs @@ -6,7 +6,7 @@ using osu.Framework.Platform; namespace osu.Game.Database { - public class DatabaseContextFactory + public class DatabaseContextFactory : IDatabaseContextFactory { private readonly GameHost host; diff --git a/osu.Game/Database/IDatabaseContextFactory.cs b/osu.Game/Database/IDatabaseContextFactory.cs new file mode 100644 index 0000000000..bc1bc0349c --- /dev/null +++ b/osu.Game/Database/IDatabaseContextFactory.cs @@ -0,0 +1,20 @@ +// Copyright (c) 2007-2018 ppy Pty Ltd . +// Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE + +namespace osu.Game.Database +{ + public interface IDatabaseContextFactory + { + /// + /// Get a context for read-only usage. + /// + OsuDbContext Get(); + + /// + /// Request a context for write usage. Can be consumed in a nested fashion (and will return the same underlying context). + /// This method may block if a write is already active on a different thread. + /// + /// A usage containing a usable context. + DatabaseWriteUsage GetForWrite(); + } +} diff --git a/osu.Game/Database/SingletonContextFactory.cs b/osu.Game/Database/SingletonContextFactory.cs index 88a43dc836..067e4fd8eb 100644 --- a/osu.Game/Database/SingletonContextFactory.cs +++ b/osu.Game/Database/SingletonContextFactory.cs @@ -3,19 +3,17 @@ namespace osu.Game.Database { - public class SingletonContextFactory : DatabaseContextFactory + public class SingletonContextFactory : IDatabaseContextFactory { private readonly OsuDbContext context; public SingletonContextFactory(OsuDbContext context) - : base(null) { this.context = context; } - protected override OsuDbContext CreateContext() - { - return context; - } + public OsuDbContext Get() => context; + + public DatabaseWriteUsage GetForWrite() => new DatabaseWriteUsage(context, null); } } diff --git a/osu.Game/IO/FileStore.cs b/osu.Game/IO/FileStore.cs index 9889088dc4..ab81ba4851 100644 --- a/osu.Game/IO/FileStore.cs +++ b/osu.Game/IO/FileStore.cs @@ -21,7 +21,7 @@ namespace osu.Game.IO public new Storage Storage => base.Storage; - public FileStore(DatabaseContextFactory contextFactory, Storage storage) : base(contextFactory, storage.GetStorageForDirectory(@"files")) + public FileStore(IDatabaseContextFactory contextFactory, Storage storage) : base(contextFactory, storage.GetStorageForDirectory(@"files")) { Store = new StorageBackedResourceStore(Storage); } diff --git a/osu.Game/Rulesets/RulesetStore.cs b/osu.Game/Rulesets/RulesetStore.cs index f66a126211..92fbf25f04 100644 --- a/osu.Game/Rulesets/RulesetStore.cs +++ b/osu.Game/Rulesets/RulesetStore.cs @@ -25,7 +25,7 @@ namespace osu.Game.Rulesets loadRulesetFromFile(file); } - public RulesetStore(DatabaseContextFactory factory) + public RulesetStore(IDatabaseContextFactory factory) : base(factory) { AddMissingRulesets(); diff --git a/osu.Game/osu.Game.csproj b/osu.Game/osu.Game.csproj index 71f1629c19..02801eb81f 100644 --- a/osu.Game/osu.Game.csproj +++ b/osu.Game/osu.Game.csproj @@ -276,6 +276,7 @@ + From 8d313486b3e03dc05e8f27327bba14fc3a7f59ba Mon Sep 17 00:00:00 2001 From: Shane Woolcock Date: Tue, 13 Feb 2018 00:40:34 +1030 Subject: [PATCH 27/33] Add a confirmation dialog to the Delete option in the beatmap context menu --- .../Select/Carousel/DrawableCarouselBeatmapSet.cs | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs b/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs index d8cfd79e12..6933f5503a 100644 --- a/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs +++ b/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs @@ -25,10 +25,10 @@ namespace osu.Game.Screens.Select.Carousel { public class DrawableCarouselBeatmapSet : DrawableCarouselItem, IHasContextMenu { - private Action deleteRequested; private Action restoreHiddenRequested; private Action viewDetails; + private DialogOverlay dialogOverlay; private readonly BeatmapSetInfo beatmapSet; public DrawableCarouselBeatmapSet(CarouselBeatmapSet set) @@ -38,13 +38,13 @@ namespace osu.Game.Screens.Select.Carousel } [BackgroundDependencyLoader(true)] - private void load(LocalisationEngine localisation, BeatmapManager manager, BeatmapSetOverlay beatmapOverlay) + private void load(LocalisationEngine localisation, BeatmapManager manager, BeatmapSetOverlay beatmapOverlay, DialogOverlay overlay) { if (localisation == null) throw new ArgumentNullException(nameof(localisation)); restoreHiddenRequested = s => s.Beatmaps.ForEach(manager.Restore); - deleteRequested = manager.Delete; + dialogOverlay = overlay; if (beatmapOverlay != null) viewDetails = beatmapOverlay.ShowBeatmapSet; @@ -89,6 +89,12 @@ namespace osu.Game.Screens.Select.Carousel }; } + private void delete(BeatmapSetInfo beatmap) + { + if (beatmap == null) return; + dialogOverlay?.Push(new BeatmapDeleteDialog(beatmap)); + } + public MenuItem[] ContextMenuItems { get @@ -104,7 +110,7 @@ namespace osu.Game.Screens.Select.Carousel if (beatmapSet.Beatmaps.Any(b => b.Hidden)) items.Add(new OsuMenuItem("Restore all hidden", MenuItemType.Standard, () => restoreHiddenRequested?.Invoke(beatmapSet))); - items.Add(new OsuMenuItem("Delete", MenuItemType.Destructive, () => deleteRequested?.Invoke(beatmapSet))); + items.Add(new OsuMenuItem("Delete", MenuItemType.Destructive, () => delete(beatmapSet))); return items.ToArray(); } From e8e093d6f2bda07dfb3cc596b553044db91c9262 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 13 Feb 2018 14:54:01 +0900 Subject: [PATCH 28/33] Fix incorrect xmldoc --- osu.Game/Beatmaps/BeatmapManager.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index 5748062fd5..be04a78034 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -535,7 +535,7 @@ namespace osu.Game.Beatmaps } /// - /// Create a SHA-2 hash from the provided archive based on contained beatmap filenames. + /// Create a SHA-2 hash from the provided archive based on contained beatmap (.osu) file content. /// private string computeBeatmapSetHash(ArchiveReader reader) { From 35613263064a696f5900b51de7bccc73139c2796 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 13 Feb 2018 14:54:46 +0900 Subject: [PATCH 29/33] Remove fixed issue --- osu.Game/Beatmaps/BeatmapManager.cs | 2 -- 1 file changed, 2 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index be04a78034..40b63ffa39 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -191,8 +191,6 @@ namespace osu.Game.Beatmaps var existingOnlineId = beatmaps.BeatmapSets.FirstOrDefault(b => b.OnlineBeatmapSetID == beatmapSet.OnlineBeatmapSetID); if (existingOnlineId != null) { - // {Microsoft.EntityFrameworkCore.DbUpdateConcurrencyException: Database operation expected to affect 1 row(s) but actually affected 0 row(s). Data may have been modified or deleted since entities were loaded. See http://go.microsoft.com/fwlink/?LinkId=527962…} - Delete(existingOnlineId); beatmaps.Cleanup(s => s.ID == existingOnlineId.ID); } From d603d032d59abcd32cbdce3bfa7503dca4541705 Mon Sep 17 00:00:00 2001 From: Shane Woolcock Date: Tue, 13 Feb 2018 16:26:05 +1030 Subject: [PATCH 30/33] Inlined delete beatmap dialog --- .../Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs b/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs index 6933f5503a..5204b7d787 100644 --- a/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs +++ b/osu.Game/Screens/Select/Carousel/DrawableCarouselBeatmapSet.cs @@ -89,12 +89,6 @@ namespace osu.Game.Screens.Select.Carousel }; } - private void delete(BeatmapSetInfo beatmap) - { - if (beatmap == null) return; - dialogOverlay?.Push(new BeatmapDeleteDialog(beatmap)); - } - public MenuItem[] ContextMenuItems { get @@ -110,7 +104,7 @@ namespace osu.Game.Screens.Select.Carousel if (beatmapSet.Beatmaps.Any(b => b.Hidden)) items.Add(new OsuMenuItem("Restore all hidden", MenuItemType.Standard, () => restoreHiddenRequested?.Invoke(beatmapSet))); - items.Add(new OsuMenuItem("Delete", MenuItemType.Destructive, () => delete(beatmapSet))); + items.Add(new OsuMenuItem("Delete", MenuItemType.Destructive, () => dialogOverlay?.Push(new BeatmapDeleteDialog(beatmapSet)))); return items.ToArray(); } From ab34123ba81fe37c62aaf8e500ec42346a17ea23 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 13 Feb 2018 14:58:15 +0900 Subject: [PATCH 31/33] Remove unnecessary class variable --- osu.Game/Database/DatabaseContextFactory.cs | 20 +++++++++----------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/osu.Game/Database/DatabaseContextFactory.cs b/osu.Game/Database/DatabaseContextFactory.cs index 002e9e456d..d8044e6eb1 100644 --- a/osu.Game/Database/DatabaseContextFactory.cs +++ b/osu.Game/Database/DatabaseContextFactory.cs @@ -16,8 +16,6 @@ namespace osu.Game.Database private readonly object writeLock = new object(); - private OsuDbContext writeContext; - private bool currentWriteDidWrite; private volatile int currentWriteUsages; @@ -43,7 +41,7 @@ namespace osu.Game.Database Interlocked.Increment(ref currentWriteUsages); - return new DatabaseWriteUsage(writeContext ?? (writeContext = threadContexts.Value), usageCompleted); + return new DatabaseWriteUsage(threadContexts.Value, usageCompleted); } private void usageCompleted(DatabaseWriteUsage usage) @@ -56,19 +54,12 @@ namespace osu.Game.Database if (usages > 0) return; - if (currentWriteDidWrite) { - writeContext.Dispose(); currentWriteDidWrite = false; - // once all writes are complete, we want to refresh thread-specific contexts to make sure they don't have stale local caches. recycleThreadContexts(); } - - // always set to null (even when a write didn't occur) so we get the correct thread context on next write request. - writeContext = null; - } finally { @@ -76,7 +67,14 @@ namespace osu.Game.Database } } - private void recycleThreadContexts() => threadContexts = new ThreadLocal(CreateContext); + private void recycleThreadContexts() + { + if (threadContexts != null) + foreach (var context in threadContexts.Values) + context.Dispose(); + + threadContexts = new ThreadLocal(CreateContext, true); + } protected virtual OsuDbContext CreateContext() { From 50cdb03cd9e55911c8727467dad7ebb18b4e35b0 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 13 Feb 2018 15:08:45 +0900 Subject: [PATCH 32/33] Don't dispose read contexts --- osu.Game/Database/DatabaseContextFactory.cs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/osu.Game/Database/DatabaseContextFactory.cs b/osu.Game/Database/DatabaseContextFactory.cs index d8044e6eb1..2068d6bd8a 100644 --- a/osu.Game/Database/DatabaseContextFactory.cs +++ b/osu.Game/Database/DatabaseContextFactory.cs @@ -56,7 +56,11 @@ namespace osu.Game.Database if (currentWriteDidWrite) { + // explicitly dispose to ensure any outstanding flushes happen as soon as possible (and underlying resources are purged). + usage.Context.Dispose(); + currentWriteDidWrite = false; + // once all writes are complete, we want to refresh thread-specific contexts to make sure they don't have stale local caches. recycleThreadContexts(); } @@ -67,14 +71,7 @@ namespace osu.Game.Database } } - private void recycleThreadContexts() - { - if (threadContexts != null) - foreach (var context in threadContexts.Values) - context.Dispose(); - - threadContexts = new ThreadLocal(CreateContext, true); - } + private void recycleThreadContexts() => threadContexts = new ThreadLocal(CreateContext); protected virtual OsuDbContext CreateContext() { From 8c42225646402eb02079d9046e07f1370fc6f3f8 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 13 Feb 2018 15:08:51 +0900 Subject: [PATCH 33/33] Fix outdated xmldoc --- osu.Game/Beatmaps/BeatmapManager.cs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index 40b63ffa39..47773528a6 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -588,11 +588,8 @@ namespace osu.Game.Beatmaps } /// - /// Import a beamap into our local storage. - /// If the beatmap is already imported, the existing instance will be returned. + /// Create all required s for the provided archive. /// - /// The beatmap archive to be read. - /// The imported beatmap, or an existing instance if it is already present. private List createBeatmapDifficulties(ArchiveReader reader) { var beatmapInfos = new List();