From 0df5432f5ebe25ea320f181c3183d3edb483b526 Mon Sep 17 00:00:00 2001 From: Aergwyn Date: Wed, 22 Nov 2017 21:45:18 +0100 Subject: [PATCH 1/9] removed line that set metadata per beatmap to null --- osu.Game/Beatmaps/BeatmapManager.cs | 3 --- 1 file changed, 3 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index f461317ce1..eb10b23c6f 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -515,9 +515,6 @@ namespace osu.Game.Beatmaps if (existing == null) { - // TODO: Diff beatmap metadata with set metadata and leave it here if necessary - beatmap.BeatmapInfo.Metadata = null; - RulesetInfo ruleset = rulesets.GetRuleset(beatmap.BeatmapInfo.RulesetID); // TODO: this should be done in a better place once we actually need to dynamically update it. From e3a230320ae1aa3b1f2279b539d0e97b416d48fc Mon Sep 17 00:00:00 2001 From: Aergwyn Date: Thu, 23 Nov 2017 19:46:58 +0100 Subject: [PATCH 2/9] compare metdata and remove duplicate from beatmap to prevent redundant storage --- osu.Game/Beatmaps/BeatmapManager.cs | 4 ++++ osu.Game/Beatmaps/BeatmapMetadata.cs | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/osu.Game/Beatmaps/BeatmapManager.cs b/osu.Game/Beatmaps/BeatmapManager.cs index eb10b23c6f..ca715b8d1e 100644 --- a/osu.Game/Beatmaps/BeatmapManager.cs +++ b/osu.Game/Beatmaps/BeatmapManager.cs @@ -515,6 +515,10 @@ namespace osu.Game.Beatmaps if (existing == null) { + // Exclude beatmap-metadata if it's equal to beatmapset-metadata + if (metadata.Equals(beatmap.Metadata)) + beatmap.BeatmapInfo.Metadata = null; + RulesetInfo ruleset = rulesets.GetRuleset(beatmap.BeatmapInfo.RulesetID); // TODO: this should be done in a better place once we actually need to dynamically update it. diff --git a/osu.Game/Beatmaps/BeatmapMetadata.cs b/osu.Game/Beatmaps/BeatmapMetadata.cs index 89f9ebf47a..2cd8e2669f 100644 --- a/osu.Game/Beatmaps/BeatmapMetadata.cs +++ b/osu.Game/Beatmaps/BeatmapMetadata.cs @@ -66,5 +66,23 @@ namespace osu.Game.Beatmaps Source, Tags }.Where(s => !string.IsNullOrEmpty(s)).ToArray(); + + public override bool Equals(object other) + { + var otherMetadata = other as BeatmapMetadata; + if (otherMetadata == null) return false; + + return (onlineBeatmapSetID?.Equals(otherMetadata.onlineBeatmapSetID) ?? false) + && (Title?.Equals(otherMetadata.Title) ?? false) + && (TitleUnicode?.Equals(otherMetadata.TitleUnicode) ?? false) + && (Artist?.Equals(otherMetadata.Artist) ?? false) + && (ArtistUnicode?.Equals(otherMetadata.ArtistUnicode) ?? false) + && (AuthorString?.Equals(otherMetadata.AuthorString) ?? false) + && (Source?.Equals(otherMetadata.Source) ?? false) + && (Tags?.Equals(otherMetadata.Tags) ?? false) + && PreviewTime.Equals(otherMetadata.PreviewTime) + && (AudioFile?.Equals(otherMetadata.AudioFile) ?? false) + && (BackgroundFile?.Equals(otherMetadata.BackgroundFile) ?? false); + } } } From 5da1466e284b584c636a5cf6f2c00c8280d04e3b Mon Sep 17 00:00:00 2001 From: Unknown Date: Sat, 25 Nov 2017 15:05:59 +0100 Subject: [PATCH 3/9] requested changes use IEquatable instead of overriding Equals and `==` operator for primitive types. --- osu.Game/Beatmaps/BeatmapMetadata.cs | 31 ++++++++++++++-------------- 1 file changed, 16 insertions(+), 15 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapMetadata.cs b/osu.Game/Beatmaps/BeatmapMetadata.cs index 2cd8e2669f..22a64820bc 100644 --- a/osu.Game/Beatmaps/BeatmapMetadata.cs +++ b/osu.Game/Beatmaps/BeatmapMetadata.cs @@ -1,6 +1,7 @@ // Copyright (c) 2007-2017 ppy Pty Ltd . // Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE +using System; using System.Collections.Generic; using System.ComponentModel.DataAnnotations.Schema; using System.Linq; @@ -9,7 +10,7 @@ using osu.Game.Users; namespace osu.Game.Beatmaps { - public class BeatmapMetadata + public class BeatmapMetadata : IEquatable { [DatabaseGenerated(DatabaseGeneratedOption.Identity)] public int ID { get; set; } @@ -67,22 +68,22 @@ namespace osu.Game.Beatmaps Tags }.Where(s => !string.IsNullOrEmpty(s)).ToArray(); - public override bool Equals(object other) + public bool Equals(BeatmapMetadata other) { - var otherMetadata = other as BeatmapMetadata; - if (otherMetadata == null) return false; + if (other == null) + return false; - return (onlineBeatmapSetID?.Equals(otherMetadata.onlineBeatmapSetID) ?? false) - && (Title?.Equals(otherMetadata.Title) ?? false) - && (TitleUnicode?.Equals(otherMetadata.TitleUnicode) ?? false) - && (Artist?.Equals(otherMetadata.Artist) ?? false) - && (ArtistUnicode?.Equals(otherMetadata.ArtistUnicode) ?? false) - && (AuthorString?.Equals(otherMetadata.AuthorString) ?? false) - && (Source?.Equals(otherMetadata.Source) ?? false) - && (Tags?.Equals(otherMetadata.Tags) ?? false) - && PreviewTime.Equals(otherMetadata.PreviewTime) - && (AudioFile?.Equals(otherMetadata.AudioFile) ?? false) - && (BackgroundFile?.Equals(otherMetadata.BackgroundFile) ?? false); + return onlineBeatmapSetID == other.onlineBeatmapSetID + && (Title?.Equals(other.Title) ?? false) + && (TitleUnicode?.Equals(other.TitleUnicode) ?? false) + && (Artist?.Equals(other.Artist) ?? false) + && (ArtistUnicode?.Equals(other.ArtistUnicode) ?? false) + && (AuthorString?.Equals(other.AuthorString) ?? false) + && (Source?.Equals(other.Source) ?? false) + && (Tags?.Equals(other.Tags) ?? false) + && PreviewTime == other.PreviewTime + && (AudioFile?.Equals(other.AudioFile) ?? false) + && (BackgroundFile?.Equals(other.BackgroundFile) ?? false); } } } From ae55d392de9a05f9d878d79a319e4a39070a1bd3 Mon Sep 17 00:00:00 2001 From: Unknown Date: Sat, 25 Nov 2017 15:28:39 +0100 Subject: [PATCH 4/9] only use `==` for comparion on primitive types --- osu.Game/Beatmaps/BeatmapMetadata.cs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapMetadata.cs b/osu.Game/Beatmaps/BeatmapMetadata.cs index 22a64820bc..a78ef25166 100644 --- a/osu.Game/Beatmaps/BeatmapMetadata.cs +++ b/osu.Game/Beatmaps/BeatmapMetadata.cs @@ -74,16 +74,16 @@ namespace osu.Game.Beatmaps return false; return onlineBeatmapSetID == other.onlineBeatmapSetID - && (Title?.Equals(other.Title) ?? false) - && (TitleUnicode?.Equals(other.TitleUnicode) ?? false) - && (Artist?.Equals(other.Artist) ?? false) - && (ArtistUnicode?.Equals(other.ArtistUnicode) ?? false) - && (AuthorString?.Equals(other.AuthorString) ?? false) - && (Source?.Equals(other.Source) ?? false) - && (Tags?.Equals(other.Tags) ?? false) + && Title == other.Title + && TitleUnicode == other.TitleUnicode + && Artist == other.Artist + && ArtistUnicode == other.ArtistUnicode + && AuthorString == other.AuthorString + && Source == other.Source + && Tags == other.Tags && PreviewTime == other.PreviewTime - && (AudioFile?.Equals(other.AudioFile) ?? false) - && (BackgroundFile?.Equals(other.BackgroundFile) ?? false); + && AudioFile == other.AudioFile + && BackgroundFile == other.BackgroundFile; } } } From d87235a2891e64708a7e617af753e420eb4efa3e Mon Sep 17 00:00:00 2001 From: Aergwyn Date: Mon, 27 Nov 2017 20:08:16 +0100 Subject: [PATCH 5/9] prevent inserting duplicate metadata --- osu.Game/Beatmaps/BeatmapStore.cs | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/osu.Game/Beatmaps/BeatmapStore.cs b/osu.Game/Beatmaps/BeatmapStore.cs index 3875202e32..ef658d2ff6 100644 --- a/osu.Game/Beatmaps/BeatmapStore.cs +++ b/osu.Game/Beatmaps/BeatmapStore.cs @@ -2,6 +2,7 @@ // Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE using System; +using System.Collections.Generic; using System.Linq; using Microsoft.EntityFrameworkCore; using osu.Game.Database; @@ -32,6 +33,15 @@ namespace osu.Game.Beatmaps { var context = GetContext(); + foreach (var beatmap in beatmapSet.Beatmaps.Where(b => b.Metadata != null)) + { + 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); context.SaveChanges(); From c058065a3ab78258ec24ef90a33738a4a8b1af69 Mon Sep 17 00:00:00 2001 From: Aergwyn Date: Mon, 27 Nov 2017 20:24:01 +0100 Subject: [PATCH 6/9] remove unnecessary using --- osu.Game/Beatmaps/BeatmapStore.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/osu.Game/Beatmaps/BeatmapStore.cs b/osu.Game/Beatmaps/BeatmapStore.cs index ef658d2ff6..352f793aac 100644 --- a/osu.Game/Beatmaps/BeatmapStore.cs +++ b/osu.Game/Beatmaps/BeatmapStore.cs @@ -2,7 +2,6 @@ // Licensed under the MIT Licence - https://raw.githubusercontent.com/ppy/osu/master/LICENCE using System; -using System.Collections.Generic; using System.Linq; using Microsoft.EntityFrameworkCore; using osu.Game.Database; From 76c09ae59ef513cf899f0ca0310252d3d0e2e8b5 Mon Sep 17 00:00:00 2001 From: Aergwyn Date: Thu, 7 Dec 2017 13:44:47 +0100 Subject: [PATCH 7/9] added comments for local context checking --- osu.Game/Beatmaps/BeatmapStore.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapStore.cs b/osu.Game/Beatmaps/BeatmapStore.cs index 352f793aac..b23f093786 100644 --- a/osu.Game/Beatmaps/BeatmapStore.cs +++ b/osu.Game/Beatmaps/BeatmapStore.cs @@ -34,11 +34,12 @@ namespace osu.Game.Beatmaps foreach (var beatmap in beatmapSet.Beatmaps.Where(b => b.Metadata != null)) { + // check local context for metadata so we can reuse duplicates from the same set var contextMetadata = context.Set().Local.SingleOrDefault(e => e.Equals(beatmap.Metadata)); if (contextMetadata != null) - beatmap.Metadata = contextMetadata; + beatmap.Metadata = contextMetadata; // reuse existing else - context.BeatmapMetadata.Attach(beatmap.Metadata); + context.BeatmapMetadata.Attach(beatmap.Metadata); // adding new to context } context.BeatmapSetInfo.Attach(beatmapSet); From 1dcbfab18e97a3c39a01ceeeb42bb1096519269e Mon Sep 17 00:00:00 2001 From: Aergwyn Date: Thu, 7 Dec 2017 13:56:37 +0100 Subject: [PATCH 8/9] removed redundant comment --- osu.Game/Beatmaps/BeatmapStore.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Beatmaps/BeatmapStore.cs b/osu.Game/Beatmaps/BeatmapStore.cs index b23f093786..ac40b3378a 100644 --- a/osu.Game/Beatmaps/BeatmapStore.cs +++ b/osu.Game/Beatmaps/BeatmapStore.cs @@ -37,7 +37,7 @@ namespace osu.Game.Beatmaps // check local context for metadata so we can reuse duplicates from the same set var contextMetadata = context.Set().Local.SingleOrDefault(e => e.Equals(beatmap.Metadata)); if (contextMetadata != null) - beatmap.Metadata = contextMetadata; // reuse existing + beatmap.Metadata = contextMetadata; else context.BeatmapMetadata.Attach(beatmap.Metadata); // adding new to context } From 95955d68eff35ed128d90aab1ec1ca532bb4e3a1 Mon Sep 17 00:00:00 2001 From: Aergwyn Date: Thu, 7 Dec 2017 14:14:50 +0100 Subject: [PATCH 9/9] rephrased description of local context checking --- osu.Game/Beatmaps/BeatmapStore.cs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/osu.Game/Beatmaps/BeatmapStore.cs b/osu.Game/Beatmaps/BeatmapStore.cs index ac40b3378a..fb45c17454 100644 --- a/osu.Game/Beatmaps/BeatmapStore.cs +++ b/osu.Game/Beatmaps/BeatmapStore.cs @@ -34,12 +34,14 @@ namespace osu.Game.Beatmaps foreach (var beatmap in beatmapSet.Beatmaps.Where(b => b.Metadata != null)) { - // check local context for metadata so we can reuse duplicates from the same set + // 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); // adding new to context + context.BeatmapMetadata.Attach(beatmap.Metadata); } context.BeatmapSetInfo.Attach(beatmapSet);