From 7e7716f942a5ef2df2e49c8783c9f1fa829614a3 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Mon, 13 Jun 2022 15:40:11 +0900 Subject: [PATCH 01/16] Support undo/redo for control points --- osu.Game/Beatmaps/Formats/LegacyDecoder.cs | 2 +- osu.Game/Screens/Edit/EditorBeatmap.cs | 59 ++++++----- .../Edit/LegacyEditorBeatmapPatcher.cs | 100 ++++++++++++------ 3 files changed, 105 insertions(+), 56 deletions(-) diff --git a/osu.Game/Beatmaps/Formats/LegacyDecoder.cs b/osu.Game/Beatmaps/Formats/LegacyDecoder.cs index ff13e6136091..25551d1ef677 100644 --- a/osu.Game/Beatmaps/Formats/LegacyDecoder.cs +++ b/osu.Game/Beatmaps/Formats/LegacyDecoder.cs @@ -143,7 +143,7 @@ protected KeyValuePair SplitKeyVal(string line, char separator = protected string CleanFilename(string path) => path.Trim('"').ToStandardisedPath(); - protected enum Section + public enum Section { General, Editor, diff --git a/osu.Game/Screens/Edit/EditorBeatmap.cs b/osu.Game/Screens/Edit/EditorBeatmap.cs index c9449f32596f..577a8291ad33 100644 --- a/osu.Game/Screens/Edit/EditorBeatmap.cs +++ b/osu.Game/Screens/Edit/EditorBeatmap.cs @@ -71,31 +71,7 @@ public class EditorBeatmap : TransactionalCommitComponent, IBeatmap, IBeatSnapPr public EditorBeatmap(IBeatmap playableBeatmap, ISkin beatmapSkin = null, BeatmapInfo beatmapInfo = null) { PlayableBeatmap = playableBeatmap; - - // ensure we are not working with legacy control points. - // if we leave the legacy points around they will be applied over any local changes on - // ApplyDefaults calls. this should eventually be removed once the default logic is moved to the decoder/converter. - if (PlayableBeatmap.ControlPointInfo is LegacyControlPointInfo) - { - var newControlPoints = new ControlPointInfo(); - - foreach (var controlPoint in PlayableBeatmap.ControlPointInfo.AllControlPoints) - { - switch (controlPoint) - { - case DifficultyControlPoint _: - case SampleControlPoint _: - // skip legacy types. - continue; - - default: - newControlPoints.Add(controlPoint.Time, controlPoint); - break; - } - } - - playableBeatmap.ControlPointInfo = newControlPoints; - } + PlayableBeatmap.ControlPointInfo = ConvertControlPoints(PlayableBeatmap.ControlPointInfo); this.beatmapInfo = beatmapInfo ?? playableBeatmap.BeatmapInfo; @@ -108,6 +84,39 @@ public EditorBeatmap(IBeatmap playableBeatmap, ISkin beatmapSkin = null, Beatmap trackStartTime(obj); } + /// + /// Converts a such that the resultant is non-legacy. + /// + /// The to convert. + /// The non-legacy . is returned if already non-legacy. + public static ControlPointInfo ConvertControlPoints(ControlPointInfo incoming) + { + // ensure we are not working with legacy control points. + // if we leave the legacy points around they will be applied over any local changes on + // ApplyDefaults calls. this should eventually be removed once the default logic is moved to the decoder/converter. + if (!(incoming is LegacyControlPointInfo)) + return incoming; + + var newControlPoints = new ControlPointInfo(); + + foreach (var controlPoint in incoming.AllControlPoints) + { + switch (controlPoint) + { + case DifficultyControlPoint _: + case SampleControlPoint _: + // skip legacy types. + continue; + + default: + newControlPoints.Add(controlPoint.Time, controlPoint); + break; + } + } + + return newControlPoints; + } + public BeatmapInfo BeatmapInfo { get => beatmapInfo; diff --git a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs index 3ed2a7efe2b0..ba9a7ed1a375 100644 --- a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs +++ b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs @@ -7,9 +7,11 @@ using System.IO; using System.Text; using DiffPlex; +using DiffPlex.Model; using osu.Framework.Audio.Track; using osu.Framework.Graphics.Textures; using osu.Game.Beatmaps; +using osu.Game.Beatmaps.Formats; using osu.Game.IO; using osu.Game.Skinning; using Decoder = osu.Game.Beatmaps.Formats.Decoder; @@ -32,61 +34,99 @@ public void Patch(byte[] currentState, byte[] newState) { // Diff the beatmaps var result = new Differ().CreateLineDiffs(readString(currentState), readString(newState), true, false); + IBeatmap newBeatmap = null; + + editorBeatmap.BeginChange(); + processHitObjects(result, () => newBeatmap ??= readBeatmap(newState)); + processTimingPoints(result, () => newBeatmap ??= readBeatmap(newState)); + editorBeatmap.EndChange(); + } + + private void processTimingPoints(DiffResult result, Func getNewBeatmap) + { + findChangedIndices(result, LegacyDecoder.Section.TimingPoints, out var removedIndices, out var addedIndices); + + if (removedIndices.Count == 0 && addedIndices.Count == 0) + return; + + // Due to conversion from legacy to non-legacy control points, it becomes difficult to diff control points correctly. + // So instead _all_ control points are reloaded if _any_ control point is changed. + + var newControlPoints = EditorBeatmap.ConvertControlPoints(getNewBeatmap().ControlPointInfo); + + editorBeatmap.ControlPointInfo.Clear(); + foreach (var point in newControlPoints.AllControlPoints) + editorBeatmap.ControlPointInfo.Add(point.Time, point); + } + + private void processHitObjects(DiffResult result, Func getNewBeatmap) + { + findChangedIndices(result, LegacyDecoder.Section.HitObjects, out var removedIndices, out var addedIndices); + + foreach (int removed in removedIndices) + editorBeatmap.RemoveAt(removed); + + if (addedIndices.Count > 0) + { + var newBeatmap = getNewBeatmap(); + + foreach (int i in addedIndices) + editorBeatmap.Insert(i, newBeatmap.HitObjects[i]); + } + } + + private void findChangedIndices(DiffResult result, LegacyDecoder.Section section, out List removedIndices, out List addedIndices) + { + removedIndices = new List(); + addedIndices = new List(); // Find the index of [HitObject] sections. Lines changed prior to this index are ignored. - int oldHitObjectsIndex = Array.IndexOf(result.PiecesOld, "[HitObjects]"); - int newHitObjectsIndex = Array.IndexOf(result.PiecesNew, "[HitObjects]"); + int oldSectionStartIndex = Array.IndexOf(result.PiecesOld, $"[{section}]"); + int oldSectionEndIndex = Array.FindIndex(result.PiecesOld, oldSectionStartIndex + 1, s => s.StartsWith(@"[", StringComparison.Ordinal)); + + if (oldSectionEndIndex == -1) + oldSectionEndIndex = result.PiecesOld.Length; + + int newSectionStartIndex = Array.IndexOf(result.PiecesNew, $"[{section}]"); + int newSectionEndIndex = Array.FindIndex(result.PiecesNew, newSectionStartIndex + 1, s => s.StartsWith(@"[", StringComparison.Ordinal)); - Debug.Assert(oldHitObjectsIndex >= 0); - Debug.Assert(newHitObjectsIndex >= 0); + if (newSectionEndIndex == -1) + newSectionEndIndex = result.PiecesOld.Length; - var toRemove = new List(); - var toAdd = new List(); + Debug.Assert(oldSectionStartIndex >= 0); + Debug.Assert(newSectionStartIndex >= 0); foreach (var block in result.DiffBlocks) { - // Removed hitobjects + // Removed indices for (int i = 0; i < block.DeleteCountA; i++) { - int hoIndex = block.DeleteStartA + i - oldHitObjectsIndex - 1; + int objectIndex = block.DeleteStartA + i; - if (hoIndex < 0) + if (objectIndex <= oldSectionStartIndex || objectIndex >= oldSectionEndIndex) continue; - toRemove.Add(hoIndex); + removedIndices.Add(objectIndex - oldSectionStartIndex - 1); } - // Added hitobjects + // Added indices for (int i = 0; i < block.InsertCountB; i++) { - int hoIndex = block.InsertStartB + i - newHitObjectsIndex - 1; + int objectIndex = block.InsertStartB + i; - if (hoIndex < 0) + if (objectIndex <= newSectionStartIndex || objectIndex >= newSectionEndIndex) continue; - toAdd.Add(hoIndex); + addedIndices.Add(objectIndex - newSectionStartIndex - 1); } } // Sort the indices to ensure that removal + insertion indices don't get jumbled up post-removal or post-insertion. // This isn't strictly required, but the differ makes no guarantees about order. - toRemove.Sort(); - toAdd.Sort(); + removedIndices.Sort(); + addedIndices.Sort(); - editorBeatmap.BeginChange(); - - // Apply the changes. - for (int i = toRemove.Count - 1; i >= 0; i--) - editorBeatmap.RemoveAt(toRemove[i]); - - if (toAdd.Count > 0) - { - IBeatmap newBeatmap = readBeatmap(newState); - foreach (int i in toAdd) - editorBeatmap.Insert(i, newBeatmap.HitObjects[i]); - } - - editorBeatmap.EndChange(); + removedIndices.Reverse(); } private string readString(byte[] state) => Encoding.UTF8.GetString(state); From 776e7c0c718e531874247c5c5dca8c0294cd2a04 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Mon, 13 Jun 2022 15:40:18 +0900 Subject: [PATCH 02/16] Work around performance issues --- osu.Game/Screens/Edit/Timing/TimingScreen.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/Edit/Timing/TimingScreen.cs b/osu.Game/Screens/Edit/Timing/TimingScreen.cs index f498aa917ec3..015e7f5bc104 100644 --- a/osu.Game/Screens/Edit/Timing/TimingScreen.cs +++ b/osu.Game/Screens/Edit/Timing/TimingScreen.cs @@ -137,8 +137,12 @@ protected override void LoadComplete() controlPointGroups.BindTo(Beatmap.ControlPointInfo.Groups); controlPointGroups.BindCollectionChanged((sender, args) => { - table.ControlGroups = controlPointGroups; - changeHandler?.SaveState(); + // This AddOnce() works around performance issues from the LegacyEditorBeatmapPatcher re-initialising all control points every undo & redo. + Scheduler.AddOnce(() => + { + table.ControlGroups = controlPointGroups; + changeHandler?.SaveState(); + }); }, true); } From 5a18547342d1c7a242052fb28ecc72159e128f0d Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Mon, 13 Jun 2022 15:58:11 +0900 Subject: [PATCH 03/16] Compare by `char` Co-authored-by: Berkan Diler --- osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs index ba9a7ed1a375..c668778be2e8 100644 --- a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs +++ b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs @@ -82,13 +82,13 @@ private void findChangedIndices(DiffResult result, LegacyDecoder.Sectio // Find the index of [HitObject] sections. Lines changed prior to this index are ignored. int oldSectionStartIndex = Array.IndexOf(result.PiecesOld, $"[{section}]"); - int oldSectionEndIndex = Array.FindIndex(result.PiecesOld, oldSectionStartIndex + 1, s => s.StartsWith(@"[", StringComparison.Ordinal)); + int oldSectionEndIndex = Array.FindIndex(result.PiecesOld, oldSectionStartIndex + 1, s => s.StartsWith('[')); if (oldSectionEndIndex == -1) oldSectionEndIndex = result.PiecesOld.Length; int newSectionStartIndex = Array.IndexOf(result.PiecesNew, $"[{section}]"); - int newSectionEndIndex = Array.FindIndex(result.PiecesNew, newSectionStartIndex + 1, s => s.StartsWith(@"[", StringComparison.Ordinal)); + int newSectionEndIndex = Array.FindIndex(result.PiecesNew, newSectionStartIndex + 1, s => s.StartsWith('[')); if (newSectionEndIndex == -1) newSectionEndIndex = result.PiecesOld.Length; From 285e5abb417e43fb8b95054edeba2d2a05d6e90d Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Mon, 13 Jun 2022 16:55:46 +0900 Subject: [PATCH 04/16] Fix incorrect fallback value --- osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs index c668778be2e8..939d31940552 100644 --- a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs +++ b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs @@ -91,7 +91,7 @@ private void findChangedIndices(DiffResult result, LegacyDecoder.Sectio int newSectionEndIndex = Array.FindIndex(result.PiecesNew, newSectionStartIndex + 1, s => s.StartsWith('[')); if (newSectionEndIndex == -1) - newSectionEndIndex = result.PiecesOld.Length; + newSectionEndIndex = result.PiecesNew.Length; Debug.Assert(oldSectionStartIndex >= 0); Debug.Assert(newSectionStartIndex >= 0); From a8286bdf0486b5c18e480afd2facf77c24714b58 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Mon, 13 Jun 2022 16:56:08 +0900 Subject: [PATCH 05/16] Fix assertion failures --- osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs index 939d31940552..b56d990d75c0 100644 --- a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs +++ b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs @@ -82,14 +82,18 @@ private void findChangedIndices(DiffResult result, LegacyDecoder.Sectio // Find the index of [HitObject] sections. Lines changed prior to this index are ignored. int oldSectionStartIndex = Array.IndexOf(result.PiecesOld, $"[{section}]"); - int oldSectionEndIndex = Array.FindIndex(result.PiecesOld, oldSectionStartIndex + 1, s => s.StartsWith('[')); + if (oldSectionStartIndex == -1) + return; + int oldSectionEndIndex = Array.FindIndex(result.PiecesOld, oldSectionStartIndex + 1, s => s.StartsWith('[')); if (oldSectionEndIndex == -1) oldSectionEndIndex = result.PiecesOld.Length; int newSectionStartIndex = Array.IndexOf(result.PiecesNew, $"[{section}]"); - int newSectionEndIndex = Array.FindIndex(result.PiecesNew, newSectionStartIndex + 1, s => s.StartsWith('[')); + if (newSectionStartIndex == -1) + return; + int newSectionEndIndex = Array.FindIndex(result.PiecesNew, newSectionStartIndex + 1, s => s.StartsWith('[')); if (newSectionEndIndex == -1) newSectionEndIndex = result.PiecesNew.Length; From c178e5d5923cdbcc6ace0adad7b475c3865f1a66 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Mon, 13 Jun 2022 16:58:43 +0900 Subject: [PATCH 06/16] Add explanatory comment --- osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs index b56d990d75c0..66ece8fcfa2b 100644 --- a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs +++ b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs @@ -130,6 +130,8 @@ private void findChangedIndices(DiffResult result, LegacyDecoder.Sectio removedIndices.Sort(); addedIndices.Sort(); + // The expected usage of this returned list is to iterate from the start to the end of the list, such that + // these indices need to appear in reverse order for the usage to not have to deal with decrementing indices. removedIndices.Reverse(); } From f680f4d26bce5cc1d3fbc07946a8256d200b6aab Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Mon, 13 Jun 2022 17:36:32 +0900 Subject: [PATCH 07/16] Apply refactorings from review --- .../Screens/Edit/LegacyEditorBeatmapPatcher.cs | 14 +++----------- 1 file changed, 3 insertions(+), 11 deletions(-) diff --git a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs index 66ece8fcfa2b..00cb95dd7996 100644 --- a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs +++ b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs @@ -3,7 +3,6 @@ using System; using System.Collections.Generic; -using System.Diagnostics; using System.IO; using System.Text; using DiffPlex; @@ -63,8 +62,8 @@ private void processHitObjects(DiffResult result, Func getNewBeatmap) { findChangedIndices(result, LegacyDecoder.Section.HitObjects, out var removedIndices, out var addedIndices); - foreach (int removed in removedIndices) - editorBeatmap.RemoveAt(removed); + for (int i = removedIndices.Count - 1; i >= 0; i--) + editorBeatmap.RemoveAt(removedIndices[i]); if (addedIndices.Count > 0) { @@ -80,7 +79,7 @@ private void findChangedIndices(DiffResult result, LegacyDecoder.Sectio removedIndices = new List(); addedIndices = new List(); - // Find the index of [HitObject] sections. Lines changed prior to this index are ignored. + // Find the start and end indices of the relevant section headers in both the old and the new beatmap file. Lines changed outside of the modified ranges are ignored. int oldSectionStartIndex = Array.IndexOf(result.PiecesOld, $"[{section}]"); if (oldSectionStartIndex == -1) return; @@ -97,9 +96,6 @@ private void findChangedIndices(DiffResult result, LegacyDecoder.Sectio if (newSectionEndIndex == -1) newSectionEndIndex = result.PiecesNew.Length; - Debug.Assert(oldSectionStartIndex >= 0); - Debug.Assert(newSectionStartIndex >= 0); - foreach (var block in result.DiffBlocks) { // Removed indices @@ -129,10 +125,6 @@ private void findChangedIndices(DiffResult result, LegacyDecoder.Sectio // This isn't strictly required, but the differ makes no guarantees about order. removedIndices.Sort(); addedIndices.Sort(); - - // The expected usage of this returned list is to iterate from the start to the end of the list, such that - // these indices need to appear in reverse order for the usage to not have to deal with decrementing indices. - removedIndices.Reverse(); } private string readString(byte[] state) => Encoding.UTF8.GetString(state); From 03ab6fc141ea1931bf877ca55fd5aeac4162cf3a Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Mon, 20 Jun 2022 14:56:04 +0900 Subject: [PATCH 08/16] Implement IEquatable on ControlPoint --- .../Beatmaps/ControlPoints/ControlPoint.cs | 15 ++++++++-- .../ControlPoints/ControlPointGroup.cs | 26 +++++++++++++---- .../ControlPoints/DifficultyControlPoint.cs | 15 ++++++++-- .../ControlPoints/EffectControlPoint.cs | 17 +++++++++-- .../ControlPoints/SampleControlPoint.cs | 16 +++++++++-- .../ControlPoints/TimingControlPoint.cs | 14 +++++++++- osu.Game/Beatmaps/Formats/LegacyDecoder.cs | 28 ++++++++++++++++--- 7 files changed, 109 insertions(+), 22 deletions(-) diff --git a/osu.Game/Beatmaps/ControlPoints/ControlPoint.cs b/osu.Game/Beatmaps/ControlPoints/ControlPoint.cs index 5f06e03509fc..fd06c898ab64 100644 --- a/osu.Game/Beatmaps/ControlPoints/ControlPoint.cs +++ b/osu.Game/Beatmaps/ControlPoints/ControlPoint.cs @@ -1,8 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - using System; using Newtonsoft.Json; using osu.Game.Graphics; @@ -11,7 +9,7 @@ namespace osu.Game.Beatmaps.ControlPoints { - public abstract class ControlPoint : IComparable, IDeepCloneable + public abstract class ControlPoint : IComparable, IDeepCloneable, IEquatable { /// /// The time at which the control point takes effect. @@ -48,5 +46,16 @@ public virtual void CopyFrom(ControlPoint other) { Time = other.Time; } + + public sealed override bool Equals(object? obj) + => obj is ControlPoint otherControlPoint + && Equals(otherControlPoint); + + public virtual bool Equals(ControlPoint? other) + => other != null + && Time == other.Time; + + // ReSharper disable once NonReadonlyMemberInGetHashCode + public override int GetHashCode() => Time.GetHashCode(); } } diff --git a/osu.Game/Beatmaps/ControlPoints/ControlPointGroup.cs b/osu.Game/Beatmaps/ControlPoints/ControlPointGroup.cs index 9df38b01eed8..db479f0e5ba0 100644 --- a/osu.Game/Beatmaps/ControlPoints/ControlPointGroup.cs +++ b/osu.Game/Beatmaps/ControlPoints/ControlPointGroup.cs @@ -1,18 +1,16 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - using System; using System.Linq; using osu.Framework.Bindables; namespace osu.Game.Beatmaps.ControlPoints { - public class ControlPointGroup : IComparable + public class ControlPointGroup : IComparable, IEquatable { - public event Action ItemAdded; - public event Action ItemRemoved; + public event Action? ItemAdded; + public event Action? ItemRemoved; /// /// The time at which the control point takes effect. @@ -48,5 +46,23 @@ public void Remove(ControlPoint point) controlPoints.Remove(point); ItemRemoved?.Invoke(point); } + + public sealed override bool Equals(object? obj) + => obj is ControlPointGroup otherGroup + && Equals(otherGroup); + + public virtual bool Equals(ControlPointGroup? other) + => other != null + && Time == other.Time + && ControlPoints.SequenceEqual(other.ControlPoints); + + public override int GetHashCode() + { + HashCode hashCode = new HashCode(); + hashCode.Add(Time); + foreach (var point in controlPoints) + hashCode.Add(point); + return hashCode.ToHashCode(); + } } } diff --git a/osu.Game/Beatmaps/ControlPoints/DifficultyControlPoint.cs b/osu.Game/Beatmaps/ControlPoints/DifficultyControlPoint.cs index 8b3d755fb626..6f624d20c9c7 100644 --- a/osu.Game/Beatmaps/ControlPoints/DifficultyControlPoint.cs +++ b/osu.Game/Beatmaps/ControlPoints/DifficultyControlPoint.cs @@ -1,8 +1,7 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - +using System; using osu.Framework.Bindables; using osu.Game.Graphics; using osuTK.Graphics; @@ -12,7 +11,7 @@ namespace osu.Game.Beatmaps.ControlPoints /// /// Note that going forward, this control point type should always be assigned directly to HitObjects. /// - public class DifficultyControlPoint : ControlPoint + public class DifficultyControlPoint : ControlPoint, IEquatable { public static readonly DifficultyControlPoint DEFAULT = new DifficultyControlPoint { @@ -51,5 +50,15 @@ public override void CopyFrom(ControlPoint other) base.CopyFrom(other); } + + public override bool Equals(ControlPoint? other) + => other is DifficultyControlPoint otherDifficultyControlPoint + && Equals(otherDifficultyControlPoint); + + public bool Equals(DifficultyControlPoint? other) + => base.Equals(other) + && SliderVelocity == other.SliderVelocity; + + public override int GetHashCode() => HashCode.Combine(base.GetHashCode(), SliderVelocity); } } diff --git a/osu.Game/Beatmaps/ControlPoints/EffectControlPoint.cs b/osu.Game/Beatmaps/ControlPoints/EffectControlPoint.cs index 35425972da1a..365d024035cd 100644 --- a/osu.Game/Beatmaps/ControlPoints/EffectControlPoint.cs +++ b/osu.Game/Beatmaps/ControlPoints/EffectControlPoint.cs @@ -1,15 +1,14 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - +using System; using osu.Framework.Bindables; using osu.Game.Graphics; using osuTK.Graphics; namespace osu.Game.Beatmaps.ControlPoints { - public class EffectControlPoint : ControlPoint + public class EffectControlPoint : ControlPoint, IEquatable { public static readonly EffectControlPoint DEFAULT = new EffectControlPoint { @@ -83,5 +82,17 @@ public override void CopyFrom(ControlPoint other) base.CopyFrom(other); } + + public override bool Equals(ControlPoint? other) + => other is EffectControlPoint otherEffectControlPoint + && Equals(otherEffectControlPoint); + + public bool Equals(EffectControlPoint? other) + => base.Equals(other) + && OmitFirstBarLine == other.OmitFirstBarLine + && ScrollSpeed == other.ScrollSpeed + && KiaiMode == other.KiaiMode; + + public override int GetHashCode() => HashCode.Combine(base.GetHashCode(), OmitFirstBarLine, ScrollSpeed, KiaiMode); } } diff --git a/osu.Game/Beatmaps/ControlPoints/SampleControlPoint.cs b/osu.Game/Beatmaps/ControlPoints/SampleControlPoint.cs index bed499ef3f4f..ab575dfeda56 100644 --- a/osu.Game/Beatmaps/ControlPoints/SampleControlPoint.cs +++ b/osu.Game/Beatmaps/ControlPoints/SampleControlPoint.cs @@ -1,8 +1,7 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - +using System; using osu.Framework.Bindables; using osu.Game.Audio; using osu.Game.Graphics; @@ -13,7 +12,7 @@ namespace osu.Game.Beatmaps.ControlPoints /// /// Note that going forward, this control point type should always be assigned directly to HitObjects. /// - public class SampleControlPoint : ControlPoint + public class SampleControlPoint : ControlPoint, IEquatable { public const string DEFAULT_BANK = "normal"; @@ -85,5 +84,16 @@ public override void CopyFrom(ControlPoint other) base.CopyFrom(other); } + + public override bool Equals(ControlPoint? other) + => other is SampleControlPoint otherSampleControlPoint + && Equals(otherSampleControlPoint); + + public bool Equals(SampleControlPoint? other) + => base.Equals(other) + && SampleBank == other.SampleBank + && SampleVolume == other.SampleVolume; + + public override int GetHashCode() => HashCode.Combine(base.GetHashCode(), SampleBank, SampleVolume); } } diff --git a/osu.Game/Beatmaps/ControlPoints/TimingControlPoint.cs b/osu.Game/Beatmaps/ControlPoints/TimingControlPoint.cs index 922439fcb8c8..87ef4e86d5a8 100644 --- a/osu.Game/Beatmaps/ControlPoints/TimingControlPoint.cs +++ b/osu.Game/Beatmaps/ControlPoints/TimingControlPoint.cs @@ -1,6 +1,7 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +using System; using osu.Framework.Bindables; using osu.Game.Beatmaps.Timing; using osu.Game.Graphics; @@ -8,7 +9,7 @@ namespace osu.Game.Beatmaps.ControlPoints { - public class TimingControlPoint : ControlPoint + public class TimingControlPoint : ControlPoint, IEquatable { /// /// The time signature at this control point. @@ -77,5 +78,16 @@ public override void CopyFrom(ControlPoint other) base.CopyFrom(other); } + + public override bool Equals(ControlPoint? other) + => other is TimingControlPoint otherTimingControlPoint + && Equals(otherTimingControlPoint); + + public bool Equals(TimingControlPoint? other) + => base.Equals(other) + && TimeSignature.Equals(other.TimeSignature) + && BeatLength.Equals(other.BeatLength); + + public override int GetHashCode() => HashCode.Combine(base.GetHashCode(), TimeSignature, BeatLength); } } diff --git a/osu.Game/Beatmaps/Formats/LegacyDecoder.cs b/osu.Game/Beatmaps/Formats/LegacyDecoder.cs index 0865561854cb..cc6a9126bd37 100644 --- a/osu.Game/Beatmaps/Formats/LegacyDecoder.cs +++ b/osu.Game/Beatmaps/Formats/LegacyDecoder.cs @@ -1,8 +1,6 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. -#nullable disable - using System; using System.Collections.Generic; using osu.Framework.Extensions; @@ -162,7 +160,7 @@ public enum Section } [Obsolete("Do not use unless you're a legacy ruleset and 100% sure.")] - public class LegacyDifficultyControlPoint : DifficultyControlPoint + public class LegacyDifficultyControlPoint : DifficultyControlPoint, IEquatable { /// /// Legacy BPM multiplier that introduces floating-point errors for rulesets that depend on it. @@ -188,9 +186,20 @@ public override void CopyFrom(ControlPoint other) BpmMultiplier = ((LegacyDifficultyControlPoint)other).BpmMultiplier; } + + public override bool Equals(ControlPoint? other) + => other is LegacyDifficultyControlPoint otherLegacyDifficultyControlPoint + && Equals(otherLegacyDifficultyControlPoint); + + public bool Equals(LegacyDifficultyControlPoint? other) + => base.Equals(other) + && BpmMultiplier == other.BpmMultiplier; + + // ReSharper disable once NonReadonlyMemberInGetHashCode + public override int GetHashCode() => HashCode.Combine(base.GetHashCode(), BpmMultiplier); } - internal class LegacySampleControlPoint : SampleControlPoint + internal class LegacySampleControlPoint : SampleControlPoint, IEquatable { public int CustomSampleBank; @@ -215,6 +224,17 @@ public override void CopyFrom(ControlPoint other) CustomSampleBank = ((LegacySampleControlPoint)other).CustomSampleBank; } + + public override bool Equals(ControlPoint? other) + => other is LegacySampleControlPoint otherLegacySampleControlPoint + && Equals(otherLegacySampleControlPoint); + + public bool Equals(LegacySampleControlPoint? other) + => base.Equals(other) + && CustomSampleBank == other.CustomSampleBank; + + // ReSharper disable once NonReadonlyMemberInGetHashCode + public override int GetHashCode() => HashCode.Combine(base.GetHashCode(), CustomSampleBank); } } } From a922ea9b0163a805c5b2108ddecd22cb6d412c4a Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Mon, 20 Jun 2022 15:28:36 +0900 Subject: [PATCH 09/16] Fix selection by directly comparing control points Previously, all control points would get replaced, which led to performance issues that was worked around in this PR. By comparing control points, we're able to get good performance without requiring the workaround. --- .../Edit/LegacyEditorBeatmapPatcher.cs | 28 +++++++++++++++---- osu.Game/Screens/Edit/Timing/TimingScreen.cs | 8 ++---- 2 files changed, 24 insertions(+), 12 deletions(-) diff --git a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs index 6b1885539637..01bc01bf5bfb 100644 --- a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs +++ b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs @@ -6,12 +6,14 @@ using System; using System.Collections.Generic; using System.IO; +using System.Linq; using System.Text; using DiffPlex; using DiffPlex.Model; using osu.Framework.Audio.Track; using osu.Framework.Graphics.Textures; using osu.Game.Beatmaps; +using osu.Game.Beatmaps.ControlPoints; using osu.Game.Beatmaps.Formats; using osu.Game.IO; using osu.Game.Skinning; @@ -50,14 +52,28 @@ private void processTimingPoints(DiffResult result, Func getNewBeatmap if (removedIndices.Count == 0 && addedIndices.Count == 0) return; - // Due to conversion from legacy to non-legacy control points, it becomes difficult to diff control points correctly. - // So instead _all_ control points are reloaded if _any_ control point is changed. + ControlPointInfo newControlPoints = EditorBeatmap.ConvertControlPoints(getNewBeatmap().ControlPointInfo); - var newControlPoints = EditorBeatmap.ConvertControlPoints(getNewBeatmap().ControlPointInfo); + // Remove all groups from the current beatmap which don't have a corresponding equal group in the new beatmap. + foreach (var oldGroup in editorBeatmap.ControlPointInfo.Groups.ToArray()) + { + var newGroup = newControlPoints.GroupAt(oldGroup.Time); + + if (!oldGroup.Equals(newGroup)) + editorBeatmap.ControlPointInfo.RemoveGroup(oldGroup); + } + + // Add all groups from the new beatmap which don't have a corresponding equal group in the old beatmap. + foreach (var newGroup in newControlPoints.Groups) + { + var oldGroup = editorBeatmap.ControlPointInfo.GroupAt(newGroup.Time); - editorBeatmap.ControlPointInfo.Clear(); - foreach (var point in newControlPoints.AllControlPoints) - editorBeatmap.ControlPointInfo.Add(point.Time, point); + if (!newGroup.Equals(oldGroup)) + { + foreach (var point in newGroup.ControlPoints) + editorBeatmap.ControlPointInfo.Add(newGroup.Time, point); + } + } } private void processHitObjects(DiffResult result, Func getNewBeatmap) diff --git a/osu.Game/Screens/Edit/Timing/TimingScreen.cs b/osu.Game/Screens/Edit/Timing/TimingScreen.cs index 27b62f110419..60cb263a7985 100644 --- a/osu.Game/Screens/Edit/Timing/TimingScreen.cs +++ b/osu.Game/Screens/Edit/Timing/TimingScreen.cs @@ -139,12 +139,8 @@ protected override void LoadComplete() controlPointGroups.BindTo(Beatmap.ControlPointInfo.Groups); controlPointGroups.BindCollectionChanged((sender, args) => { - // This AddOnce() works around performance issues from the LegacyEditorBeatmapPatcher re-initialising all control points every undo & redo. - Scheduler.AddOnce(() => - { - table.ControlGroups = controlPointGroups; - changeHandler?.SaveState(); - }); + table.ControlGroups = controlPointGroups; + changeHandler?.SaveState(); }, true); } From ca287d093621d3997e1f8abb1b1b60e33a026e94 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Mon, 20 Jun 2022 16:38:11 +0900 Subject: [PATCH 10/16] Fix group deselected when table is recreated --- osu.Game/Screens/Edit/EditorTable.cs | 5 ++--- osu.Game/Screens/Edit/Timing/ControlPointTable.cs | 1 + 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/osu.Game/Screens/Edit/EditorTable.cs b/osu.Game/Screens/Edit/EditorTable.cs index 8765dae384f4..a290cce708f0 100644 --- a/osu.Game/Screens/Edit/EditorTable.cs +++ b/osu.Game/Screens/Edit/EditorTable.cs @@ -97,7 +97,7 @@ public RowBackground(object item) [BackgroundDependencyLoader] private void load(OverlayColourProvider colours) { - hoveredBackground.Colour = colourHover = colours.Background1; + colourHover = colours.Background1; colourSelected = colours.Colour3; } @@ -105,8 +105,7 @@ protected override void LoadComplete() { base.LoadComplete(); - // Reduce flicker of rows when offset is being changed rapidly. - // Probably need to reconsider this. + updateState(); FinishTransforms(true); } diff --git a/osu.Game/Screens/Edit/Timing/ControlPointTable.cs b/osu.Game/Screens/Edit/Timing/ControlPointTable.cs index 88c738c1d206..6d46b429f080 100644 --- a/osu.Game/Screens/Edit/Timing/ControlPointTable.cs +++ b/osu.Game/Screens/Edit/Timing/ControlPointTable.cs @@ -44,6 +44,7 @@ public IEnumerable ControlGroups { BackgroundFlow.Add(new RowBackground(group) { + Selected = group.Equals(selectedGroup?.Value), Action = () => { selectedGroup.Value = group; From 16281f4a4831cebda36a6bf669ed59ab99408b40 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Mon, 20 Jun 2022 16:52:01 +0900 Subject: [PATCH 11/16] Properly annotate method to allow null --- osu.Game/Beatmaps/ControlPoints/ControlPoint.cs | 2 +- osu.Game/Beatmaps/ControlPoints/DifficultyControlPoint.cs | 2 +- osu.Game/Beatmaps/ControlPoints/EffectControlPoint.cs | 2 +- osu.Game/Beatmaps/ControlPoints/SampleControlPoint.cs | 2 +- osu.Game/Beatmaps/ControlPoints/TimingControlPoint.cs | 2 +- osu.Game/Beatmaps/Formats/LegacyDecoder.cs | 2 +- 6 files changed, 6 insertions(+), 6 deletions(-) diff --git a/osu.Game/Beatmaps/ControlPoints/ControlPoint.cs b/osu.Game/Beatmaps/ControlPoints/ControlPoint.cs index fd06c898ab64..b91d60984938 100644 --- a/osu.Game/Beatmaps/ControlPoints/ControlPoint.cs +++ b/osu.Game/Beatmaps/ControlPoints/ControlPoint.cs @@ -28,7 +28,7 @@ public abstract class ControlPoint : IComparable, IDeepCloneable /// An existing control point to compare with. /// Whether this is redundant when placed alongside . - public abstract bool IsRedundant(ControlPoint existing); + public abstract bool IsRedundant(ControlPoint? existing); /// /// Create an unbound copy of this control point. diff --git a/osu.Game/Beatmaps/ControlPoints/DifficultyControlPoint.cs b/osu.Game/Beatmaps/ControlPoints/DifficultyControlPoint.cs index 6f624d20c9c7..c199d1da5905 100644 --- a/osu.Game/Beatmaps/ControlPoints/DifficultyControlPoint.cs +++ b/osu.Game/Beatmaps/ControlPoints/DifficultyControlPoint.cs @@ -40,7 +40,7 @@ public double SliderVelocity set => SliderVelocityBindable.Value = value; } - public override bool IsRedundant(ControlPoint existing) + public override bool IsRedundant(ControlPoint? existing) => existing is DifficultyControlPoint existingDifficulty && SliderVelocity == existingDifficulty.SliderVelocity; diff --git a/osu.Game/Beatmaps/ControlPoints/EffectControlPoint.cs b/osu.Game/Beatmaps/ControlPoints/EffectControlPoint.cs index 365d024035cd..ead07b4eaac7 100644 --- a/osu.Game/Beatmaps/ControlPoints/EffectControlPoint.cs +++ b/osu.Game/Beatmaps/ControlPoints/EffectControlPoint.cs @@ -67,7 +67,7 @@ public bool KiaiMode set => KiaiModeBindable.Value = value; } - public override bool IsRedundant(ControlPoint existing) + public override bool IsRedundant(ControlPoint? existing) => !OmitFirstBarLine && existing is EffectControlPoint existingEffect && KiaiMode == existingEffect.KiaiMode diff --git a/osu.Game/Beatmaps/ControlPoints/SampleControlPoint.cs b/osu.Game/Beatmaps/ControlPoints/SampleControlPoint.cs index ab575dfeda56..78dec679374d 100644 --- a/osu.Game/Beatmaps/ControlPoints/SampleControlPoint.cs +++ b/osu.Game/Beatmaps/ControlPoints/SampleControlPoint.cs @@ -72,7 +72,7 @@ public int SampleVolume public virtual HitSampleInfo ApplyTo(HitSampleInfo hitSampleInfo) => hitSampleInfo.With(newBank: hitSampleInfo.Bank ?? SampleBank, newVolume: hitSampleInfo.Volume > 0 ? hitSampleInfo.Volume : SampleVolume); - public override bool IsRedundant(ControlPoint existing) + public override bool IsRedundant(ControlPoint? existing) => existing is SampleControlPoint existingSample && SampleBank == existingSample.SampleBank && SampleVolume == existingSample.SampleVolume; diff --git a/osu.Game/Beatmaps/ControlPoints/TimingControlPoint.cs b/osu.Game/Beatmaps/ControlPoints/TimingControlPoint.cs index 87ef4e86d5a8..23d4d10fd8ce 100644 --- a/osu.Game/Beatmaps/ControlPoints/TimingControlPoint.cs +++ b/osu.Game/Beatmaps/ControlPoints/TimingControlPoint.cs @@ -69,7 +69,7 @@ public double BeatLength public double BPM => 60000 / BeatLength; // Timing points are never redundant as they can change the time signature. - public override bool IsRedundant(ControlPoint existing) => false; + public override bool IsRedundant(ControlPoint? existing) => false; public override void CopyFrom(ControlPoint other) { diff --git a/osu.Game/Beatmaps/Formats/LegacyDecoder.cs b/osu.Game/Beatmaps/Formats/LegacyDecoder.cs index cc6a9126bd37..a5e6ac0a1c80 100644 --- a/osu.Game/Beatmaps/Formats/LegacyDecoder.cs +++ b/osu.Game/Beatmaps/Formats/LegacyDecoder.cs @@ -213,7 +213,7 @@ public override HitSampleInfo ApplyTo(HitSampleInfo hitSampleInfo) return baseInfo; } - public override bool IsRedundant(ControlPoint existing) + public override bool IsRedundant(ControlPoint? existing) => base.IsRedundant(existing) && existing is LegacySampleControlPoint existingSample && CustomSampleBank == existingSample.CustomSampleBank; From e0c82d11ab339009718b880c95697ef74ede594c Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Mon, 20 Jun 2022 16:53:03 +0900 Subject: [PATCH 12/16] Convert == usages to ReferenceEquals --- osu.Game.Rulesets.Osu/Mods/OsuModTarget.cs | 2 +- osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs | 8 ++++---- .../Visual/UserInterface/TestSceneBeatSyncedContainer.cs | 4 ++-- osu.Game/Graphics/Containers/BeatSyncedContainer.cs | 2 +- osu.Game/Rulesets/Objects/HitObject.cs | 4 ++-- .../Timelines/Summary/Parts/ControlPointPart.cs | 2 +- .../Components/Timeline/TimelineControlPointDisplay.cs | 2 +- .../Components/Timeline/TimelineHitObjectBlueprint.cs | 6 +++--- osu.Game/Screens/Edit/EditorClock.cs | 2 +- osu.Game/Screens/Edit/Timing/ControlPointTable.cs | 3 ++- osu.Game/Screens/Edit/Timing/TimingScreen.cs | 2 +- osu.Game/Screens/Edit/Timing/WaveformComparisonDisplay.cs | 2 +- 12 files changed, 20 insertions(+), 19 deletions(-) diff --git a/osu.Game.Rulesets.Osu/Mods/OsuModTarget.cs b/osu.Game.Rulesets.Osu/Mods/OsuModTarget.cs index 4589a8eca1cf..f03bcffdc878 100644 --- a/osu.Game.Rulesets.Osu/Mods/OsuModTarget.cs +++ b/osu.Game.Rulesets.Osu/Mods/OsuModTarget.cs @@ -374,7 +374,7 @@ private IEnumerable getBeatsForTimingPoint(TimingControlPoint timingPoin int i = 0; double currentTime = timingPoint.Time; - while (!definitelyBigger(currentTime, mapEndTime) && controlPointInfo.TimingPointAt(currentTime) == timingPoint) + while (!definitelyBigger(currentTime, mapEndTime) && ReferenceEquals(controlPointInfo.TimingPointAt(currentTime), timingPoint)) { beats.Add(Math.Floor(currentTime)); i++; diff --git a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs index cf1246ef0733..e9bbf1e33d23 100644 --- a/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs +++ b/osu.Game.Tests/Visual/Editing/TestSceneEditorSaving.cs @@ -117,8 +117,8 @@ public void TestHitObjectPlacement() // After placement these must be non-default as defaults are read-only. AddAssert("Placed object has non-default control points", () => - EditorBeatmap.HitObjects[0].SampleControlPoint != SampleControlPoint.DEFAULT && - EditorBeatmap.HitObjects[0].DifficultyControlPoint != DifficultyControlPoint.DEFAULT); + !ReferenceEquals(EditorBeatmap.HitObjects[0].SampleControlPoint, SampleControlPoint.DEFAULT) && + !ReferenceEquals(EditorBeatmap.HitObjects[0].DifficultyControlPoint, DifficultyControlPoint.DEFAULT)); ReloadEditorToSameBeatmap(); @@ -126,8 +126,8 @@ public void TestHitObjectPlacement() // After placement these must be non-default as defaults are read-only. AddAssert("Placed object still has non-default control points", () => - EditorBeatmap.HitObjects[0].SampleControlPoint != SampleControlPoint.DEFAULT && - EditorBeatmap.HitObjects[0].DifficultyControlPoint != DifficultyControlPoint.DEFAULT); + !ReferenceEquals(EditorBeatmap.HitObjects[0].SampleControlPoint, SampleControlPoint.DEFAULT) && + !ReferenceEquals(EditorBeatmap.HitObjects[0].DifficultyControlPoint, DifficultyControlPoint.DEFAULT)); } [Test] diff --git a/osu.Game.Tests/Visual/UserInterface/TestSceneBeatSyncedContainer.cs b/osu.Game.Tests/Visual/UserInterface/TestSceneBeatSyncedContainer.cs index 27c107a2db36..6cf66963a537 100644 --- a/osu.Game.Tests/Visual/UserInterface/TestSceneBeatSyncedContainer.cs +++ b/osu.Game.Tests/Visual/UserInterface/TestSceneBeatSyncedContainer.cs @@ -269,7 +269,7 @@ public TestBeatSyncedContainer() private TimingControlPoint getNextTimingPoint(TimingControlPoint current) { - if (timingPoints[^1] == current) + if (ReferenceEquals(timingPoints[^1], current)) return current; int index = timingPoints.IndexOf(current); // -1 means that this is a "default beat" @@ -281,7 +281,7 @@ private int calculateBeatCount(TimingControlPoint current) { if (timingPoints.Count == 0) return 0; - if (timingPoints[^1] == current) + if (ReferenceEquals(timingPoints[^1], current)) { Debug.Assert(BeatSyncSource.Clock != null); diff --git a/osu.Game/Graphics/Containers/BeatSyncedContainer.cs b/osu.Game/Graphics/Containers/BeatSyncedContainer.cs index 4146bbcc5e97..45a935d16553 100644 --- a/osu.Game/Graphics/Containers/BeatSyncedContainer.cs +++ b/osu.Game/Graphics/Containers/BeatSyncedContainer.cs @@ -127,7 +127,7 @@ protected override void Update() TimeSinceLastBeat = beatLength - TimeUntilNextBeat; - if (timingPoint == lastTimingPoint && beatIndex == lastBeat) + if (ReferenceEquals(timingPoint, lastTimingPoint) && beatIndex == lastBeat) return; // as this event is sometimes used for sound triggers where `BeginDelayedSequence` has no effect, avoid firing it if too far away from the beat. diff --git a/osu.Game/Rulesets/Objects/HitObject.cs b/osu.Game/Rulesets/Objects/HitObject.cs index 30b8fe965ae1..d20e0616e5d2 100644 --- a/osu.Game/Rulesets/Objects/HitObject.cs +++ b/osu.Game/Rulesets/Objects/HitObject.cs @@ -108,7 +108,7 @@ public void ApplyDefaults(ControlPointInfo controlPointInfo, IBeatmapDifficultyI if (legacyInfo != null) DifficultyControlPoint = (DifficultyControlPoint)legacyInfo.DifficultyPointAt(StartTime).DeepClone(); - else if (DifficultyControlPoint == DifficultyControlPoint.DEFAULT) + else if (ReferenceEquals(DifficultyControlPoint, DifficultyControlPoint.DEFAULT)) DifficultyControlPoint = new DifficultyControlPoint(); DifficultyControlPoint.Time = StartTime; @@ -118,7 +118,7 @@ public void ApplyDefaults(ControlPointInfo controlPointInfo, IBeatmapDifficultyI // This is done here after ApplyDefaultsToSelf as we may require custom defaults to be applied to have an accurate end time. if (legacyInfo != null) SampleControlPoint = (SampleControlPoint)legacyInfo.SamplePointAt(this.GetEndTime() + control_point_leniency).DeepClone(); - else if (SampleControlPoint == SampleControlPoint.DEFAULT) + else if (ReferenceEquals(SampleControlPoint, SampleControlPoint.DEFAULT)) SampleControlPoint = new SampleControlPoint(); SampleControlPoint.Time = this.GetEndTime() + control_point_leniency; diff --git a/osu.Game/Screens/Edit/Components/Timelines/Summary/Parts/ControlPointPart.cs b/osu.Game/Screens/Edit/Components/Timelines/Summary/Parts/ControlPointPart.cs index 4e2c0183304b..ee82ded39da0 100644 --- a/osu.Game/Screens/Edit/Components/Timelines/Summary/Parts/ControlPointPart.cs +++ b/osu.Game/Screens/Edit/Components/Timelines/Summary/Parts/ControlPointPart.cs @@ -49,7 +49,7 @@ protected override void LoadBeatmap(EditorBeatmap beatmap) case NotifyCollectionChangedAction.Remove: foreach (var group in args.OldItems.OfType()) { - var matching = Children.SingleOrDefault(gv => gv.Group == group); + var matching = Children.SingleOrDefault(gv => ReferenceEquals(gv.Group, group)); if (matching != null) matching.Expire(); diff --git a/osu.Game/Screens/Edit/Compose/Components/Timeline/TimelineControlPointDisplay.cs b/osu.Game/Screens/Edit/Compose/Components/Timeline/TimelineControlPointDisplay.cs index 5d5681ea2b22..544c3f83690c 100644 --- a/osu.Game/Screens/Edit/Compose/Components/Timeline/TimelineControlPointDisplay.cs +++ b/osu.Game/Screens/Edit/Compose/Components/Timeline/TimelineControlPointDisplay.cs @@ -40,7 +40,7 @@ protected override void LoadBeatmap(EditorBeatmap beatmap) case NotifyCollectionChangedAction.Remove: foreach (var group in args.OldItems.OfType()) { - var matching = Children.SingleOrDefault(gv => gv.Group == group); + var matching = Children.SingleOrDefault(gv => ReferenceEquals(gv.Group, group)); matching?.Expire(); } diff --git a/osu.Game/Screens/Edit/Compose/Components/Timeline/TimelineHitObjectBlueprint.cs b/osu.Game/Screens/Edit/Compose/Components/Timeline/TimelineHitObjectBlueprint.cs index 34abe81ae260..32ebc9c3c1ad 100644 --- a/osu.Game/Screens/Edit/Compose/Components/Timeline/TimelineHitObjectBlueprint.cs +++ b/osu.Game/Screens/Edit/Compose/Components/Timeline/TimelineHitObjectBlueprint.cs @@ -205,7 +205,7 @@ protected override void Update() updateRepeats(repeats); } - if (difficultyControlPoint != Item.DifficultyControlPoint) + if (!ReferenceEquals(difficultyControlPoint, Item.DifficultyControlPoint)) { difficultyControlPoint = Item.DifficultyControlPoint; difficultyOverrideDisplay?.Expire(); @@ -220,7 +220,7 @@ protected override void Update() } } - if (sampleControlPoint != Item.SampleControlPoint) + if (!ReferenceEquals(sampleControlPoint, Item.SampleControlPoint)) { sampleControlPoint = Item.SampleControlPoint; sampleOverrideDisplay?.Expire(); @@ -393,7 +393,7 @@ protected override void OnDrag(DragEvent e) if (e.CurrentState.Keyboard.ShiftPressed) { - if (hitObject.DifficultyControlPoint == DifficultyControlPoint.DEFAULT) + if (ReferenceEquals(hitObject.DifficultyControlPoint, DifficultyControlPoint.DEFAULT)) hitObject.DifficultyControlPoint = new DifficultyControlPoint(); double newVelocity = hitObject.DifficultyControlPoint.SliderVelocity * (repeatHitObject.Duration / proposedDuration); diff --git a/osu.Game/Screens/Edit/EditorClock.cs b/osu.Game/Screens/Edit/EditorClock.cs index c7df36be77f4..c3b29afd307a 100644 --- a/osu.Game/Screens/Edit/EditorClock.cs +++ b/osu.Game/Screens/Edit/EditorClock.cs @@ -141,7 +141,7 @@ private void seek(int direction, bool snapped, double amount = 1) seekTime = timingPoint.Time + closestBeat * seekAmount; } - if (seekTime < timingPoint.Time && timingPoint != ControlPointInfo.TimingPoints.First()) + if (seekTime < timingPoint.Time && !ReferenceEquals(timingPoint, ControlPointInfo.TimingPoints.First())) seekTime = timingPoint.Time; SeekSmoothlyTo(seekTime); diff --git a/osu.Game/Screens/Edit/Timing/ControlPointTable.cs b/osu.Game/Screens/Edit/Timing/ControlPointTable.cs index 6d46b429f080..b6e3e91c981e 100644 --- a/osu.Game/Screens/Edit/Timing/ControlPointTable.cs +++ b/osu.Game/Screens/Edit/Timing/ControlPointTable.cs @@ -65,7 +65,8 @@ protected override void LoadComplete() selectedGroup.BindValueChanged(group => { // TODO: This should scroll the selected row into view. - foreach (var b in BackgroundFlow) b.Selected = b.Item == group.NewValue; + foreach (var b in BackgroundFlow) + b.Selected = ReferenceEquals(b.Item, group.NewValue); }, true); } diff --git a/osu.Game/Screens/Edit/Timing/TimingScreen.cs b/osu.Game/Screens/Edit/Timing/TimingScreen.cs index 60cb263a7985..c33dcc6e9150 100644 --- a/osu.Game/Screens/Edit/Timing/TimingScreen.cs +++ b/osu.Game/Screens/Edit/Timing/TimingScreen.cs @@ -222,7 +222,7 @@ private void addNew() // Try and create matching types from the currently selected control point. var selected = selectedGroup.Value; - if (selected != null && selected != group) + if (selected != null && !ReferenceEquals(selected, group)) { foreach (var controlPoint in selected.ControlPoints) { diff --git a/osu.Game/Screens/Edit/Timing/WaveformComparisonDisplay.cs b/osu.Game/Screens/Edit/Timing/WaveformComparisonDisplay.cs index 67b7abb25b4d..1cdcfdf167b7 100644 --- a/osu.Game/Screens/Edit/Timing/WaveformComparisonDisplay.cs +++ b/osu.Game/Screens/Edit/Timing/WaveformComparisonDisplay.cs @@ -128,7 +128,7 @@ private void updateTimingGroup() double? offsetChange = newStartTime - selectedGroupStartTime; var nextGroup = editorBeatmap.ControlPointInfo.TimingPoints - .SkipWhile(g => g != tcp) + .SkipWhile(g => !ReferenceEquals(g, tcp)) .Skip(1) .FirstOrDefault(); From 6a2646168334d55582a380860aaa6e61270e6f45 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Tue, 21 Jun 2022 12:05:28 +0900 Subject: [PATCH 13/16] Compare by reference in ControlPoint.Equals() --- osu.Game/Beatmaps/ControlPoints/ControlPoint.cs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/osu.Game/Beatmaps/ControlPoints/ControlPoint.cs b/osu.Game/Beatmaps/ControlPoints/ControlPoint.cs index b91d60984938..56a432aec43a 100644 --- a/osu.Game/Beatmaps/ControlPoints/ControlPoint.cs +++ b/osu.Game/Beatmaps/ControlPoints/ControlPoint.cs @@ -52,8 +52,12 @@ public sealed override bool Equals(object? obj) && Equals(otherControlPoint); public virtual bool Equals(ControlPoint? other) - => other != null - && Time == other.Time; + { + if (ReferenceEquals(other, null)) return false; + if (ReferenceEquals(other, this)) return true; + + return Time == other.Time; + } // ReSharper disable once NonReadonlyMemberInGetHashCode public override int GetHashCode() => Time.GetHashCode(); From 9763a583924d0a68d3c70e07a144d480f8bf9b01 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Tue, 21 Jun 2022 12:05:52 +0900 Subject: [PATCH 14/16] Change to use ReferenceEquals --- osu.Game/Screens/Edit/Timing/ControlPointTable.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Edit/Timing/ControlPointTable.cs b/osu.Game/Screens/Edit/Timing/ControlPointTable.cs index b6e3e91c981e..b92395727304 100644 --- a/osu.Game/Screens/Edit/Timing/ControlPointTable.cs +++ b/osu.Game/Screens/Edit/Timing/ControlPointTable.cs @@ -44,7 +44,7 @@ public IEnumerable ControlGroups { BackgroundFlow.Add(new RowBackground(group) { - Selected = group.Equals(selectedGroup?.Value), + Selected = ReferenceEquals(group, selectedGroup?.Value), Action = () => { selectedGroup.Value = group; From 93ce6fc98149a28ecc45de1a6dfba7be38fc6ec7 Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Tue, 21 Jun 2022 12:11:44 +0900 Subject: [PATCH 15/16] Remove redundant diff processing --- osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs | 9 ++------- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs index 01bc01bf5bfb..b4647c2b64bf 100644 --- a/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs +++ b/osu.Game/Screens/Edit/LegacyEditorBeatmapPatcher.cs @@ -41,17 +41,12 @@ public void Patch(byte[] currentState, byte[] newState) editorBeatmap.BeginChange(); processHitObjects(result, () => newBeatmap ??= readBeatmap(newState)); - processTimingPoints(result, () => newBeatmap ??= readBeatmap(newState)); + processTimingPoints(() => newBeatmap ??= readBeatmap(newState)); editorBeatmap.EndChange(); } - private void processTimingPoints(DiffResult result, Func getNewBeatmap) + private void processTimingPoints(Func getNewBeatmap) { - findChangedIndices(result, LegacyDecoder.Section.TimingPoints, out var removedIndices, out var addedIndices); - - if (removedIndices.Count == 0 && addedIndices.Count == 0) - return; - ControlPointInfo newControlPoints = EditorBeatmap.ConvertControlPoints(getNewBeatmap().ControlPointInfo); // Remove all groups from the current beatmap which don't have a corresponding equal group in the new beatmap. From 046b848bcd1c37a18b8ddf2141642a28668f5a6e Mon Sep 17 00:00:00 2001 From: Dan Balasescu Date: Tue, 21 Jun 2022 12:53:06 +0900 Subject: [PATCH 16/16] Split group selection to separate method --- osu.Game/Screens/Edit/Timing/ControlPointTable.cs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/osu.Game/Screens/Edit/Timing/ControlPointTable.cs b/osu.Game/Screens/Edit/Timing/ControlPointTable.cs index b92395727304..b5c162ab1108 100644 --- a/osu.Game/Screens/Edit/Timing/ControlPointTable.cs +++ b/osu.Game/Screens/Edit/Timing/ControlPointTable.cs @@ -44,7 +44,6 @@ public IEnumerable ControlGroups { BackgroundFlow.Add(new RowBackground(group) { - Selected = ReferenceEquals(group, selectedGroup?.Value), Action = () => { selectedGroup.Value = group; @@ -55,6 +54,8 @@ public IEnumerable ControlGroups Columns = createHeaders(); Content = value.Select(createContent).ToArray().ToRectangular(); + + updateSelectedGroup(); } } @@ -65,11 +66,17 @@ protected override void LoadComplete() selectedGroup.BindValueChanged(group => { // TODO: This should scroll the selected row into view. - foreach (var b in BackgroundFlow) - b.Selected = ReferenceEquals(b.Item, group.NewValue); + updateSelectedGroup(); }, true); } + private void updateSelectedGroup() + { + // TODO: This should scroll the selected row into view. + foreach (var b in BackgroundFlow) + b.Selected = ReferenceEquals(b.Item, selectedGroup?.Value); + } + private TableColumn[] createHeaders() { var columns = new List