From 58f663bb72c9950593ed02494c6053b74ab3b8f1 Mon Sep 17 00:00:00 2001 From: andy840119 Date: Sat, 30 Jul 2022 15:36:36 +0800 Subject: [PATCH 1/6] Implement the `IDeepCloneable` in the interface and use it in the utils. Also add the stupid test case to check that some of the copy/copied bindable properties should not be the same instance. --- .../Objects/NoteTest.cs | 42 +++++++++++++++++++ osu.Game.Rulesets.Karaoke/Objects/Note.cs | 20 ++++++++- osu.Game.Rulesets.Karaoke/Utils/NoteUtils.cs | 20 ++++----- 3 files changed, 69 insertions(+), 13 deletions(-) create mode 100644 osu.Game.Rulesets.Karaoke.Tests/Objects/NoteTest.cs diff --git a/osu.Game.Rulesets.Karaoke.Tests/Objects/NoteTest.cs b/osu.Game.Rulesets.Karaoke.Tests/Objects/NoteTest.cs new file mode 100644 index 000000000..b9639b021 --- /dev/null +++ b/osu.Game.Rulesets.Karaoke.Tests/Objects/NoteTest.cs @@ -0,0 +1,42 @@ +// Copyright (c) andy840119 . Licensed under the GPL Licence. +// See the LICENCE file in the repository root for full licence text. + +using NUnit.Framework; +using osu.Game.Rulesets.Karaoke.Objects; + +namespace osu.Game.Rulesets.Karaoke.Tests.Objects +{ + public class NoteTest + { + [TestCase] + public void TestClone() + { + var note = new Note + { + Text = "lyric", + }; + + var clonedNote = note.DeepClone(); + + Assert.AreNotSame(clonedNote.TextBindable, note.TextBindable); + Assert.AreEqual(clonedNote.Text, note.Text); + + Assert.AreNotSame(clonedNote.RubyTextBindable, note.RubyTextBindable); + Assert.AreEqual(clonedNote.RubyText, note.RubyText); + + Assert.AreNotSame(clonedNote.DisplayBindable, note.DisplayBindable); + Assert.AreEqual(clonedNote.Display, note.Display); + + Assert.AreNotSame(clonedNote.ToneBindable, note.ToneBindable); + Assert.AreEqual(clonedNote.Tone, note.Tone); + + Assert.AreEqual(clonedNote.Duration, note.Duration); + + Assert.AreEqual(clonedNote.StartIndex, note.StartIndex); + + Assert.AreEqual(clonedNote.EndIndex, note.EndIndex); + + Assert.AreSame(clonedNote.ParentLyric, note.ParentLyric); + } + } +} diff --git a/osu.Game.Rulesets.Karaoke/Objects/Note.cs b/osu.Game.Rulesets.Karaoke/Objects/Note.cs index 98c8eadc8..f6876c1ef 100644 --- a/osu.Game.Rulesets.Karaoke/Objects/Note.cs +++ b/osu.Game.Rulesets.Karaoke/Objects/Note.cs @@ -10,10 +10,11 @@ using osu.Game.Rulesets.Karaoke.Scoring; using osu.Game.Rulesets.Objects.Types; using osu.Game.Rulesets.Scoring; +using osu.Game.Utils; namespace osu.Game.Rulesets.Karaoke.Objects { - public class Note : KaraokeHitObject, IHasDuration, IHasText + public class Note : KaraokeHitObject, IHasDuration, IHasText, IDeepCloneable { [JsonIgnore] public readonly Bindable TextBindable = new(); @@ -110,5 +111,22 @@ public Lyric ParentLyric public override Judgement CreateJudgement() => new KaraokeNoteJudgement(); protected override HitWindows CreateHitWindows() => new KaraokeNoteHitWindows(); + + public Note DeepClone() + { + // (Note)MemberwiseClone(); + + return new() + { + Text = Text, + RubyText = RubyText, + Display = Display, + Tone = Tone, + Duration = Duration, + StartIndex = StartIndex, + EndIndex = EndIndex, + ParentLyric = ParentLyric + }; + } } } diff --git a/osu.Game.Rulesets.Karaoke/Utils/NoteUtils.cs b/osu.Game.Rulesets.Karaoke/Utils/NoteUtils.cs index 7e55b3585..46fe2cd73 100644 --- a/osu.Game.Rulesets.Karaoke/Utils/NoteUtils.cs +++ b/osu.Game.Rulesets.Karaoke/Utils/NoteUtils.cs @@ -19,18 +19,14 @@ public static Note SliceNote(Note note, double startPercentage, double durationP return CopyByTime(note, startTime, duration); } - public static Note CopyByTime(Note originNote, double startTime, double duration) => - new() - { - StartTime = startTime, - Duration = duration, - StartIndex = originNote.StartIndex, - EndIndex = originNote.EndIndex, - Text = originNote.Text, - Display = originNote.Display, - Tone = originNote.Tone, - ParentLyric = originNote.ParentLyric - }; + public static Note CopyByTime(Note originNote, double startTime, double duration) + { + var note = originNote.DeepClone(); + note.StartTime = startTime; + note.Duration = duration; + + return note; + } /// /// Get the display text while gameplay or in editor. From 3b2b5515374a10d7006bb19c89c2ca1feea343af Mon Sep 17 00:00:00 2001 From: andy840119 Date: Sat, 30 Jul 2022 16:10:10 +0800 Subject: [PATCH 2/6] Use json serializer to copy the object to make sure that all the properties in the base class are copied. --- .../Objects/NoteTest.cs | 12 +++++++++++- osu.Game.Rulesets.Karaoke/Objects/Note.cs | 18 +++++------------- 2 files changed, 16 insertions(+), 14 deletions(-) diff --git a/osu.Game.Rulesets.Karaoke.Tests/Objects/NoteTest.cs b/osu.Game.Rulesets.Karaoke.Tests/Objects/NoteTest.cs index b9639b021..87bde678b 100644 --- a/osu.Game.Rulesets.Karaoke.Tests/Objects/NoteTest.cs +++ b/osu.Game.Rulesets.Karaoke.Tests/Objects/NoteTest.cs @@ -11,9 +11,16 @@ public class NoteTest [TestCase] public void TestClone() { + var parentLyric = new Lyric(); + var note = new Note { - Text = "lyric", + Text = "ノート", + RubyText = "Note", + Display = true, + StartTime = 1000, + Duration = 500, + ParentLyric = parentLyric }; var clonedNote = note.DeepClone(); @@ -30,6 +37,9 @@ public void TestClone() Assert.AreNotSame(clonedNote.ToneBindable, note.ToneBindable); Assert.AreEqual(clonedNote.Tone, note.Tone); + Assert.AreNotSame(clonedNote.StartTimeBindable, note.StartTimeBindable); + Assert.AreEqual(clonedNote.StartTime, note.StartTime); + Assert.AreEqual(clonedNote.Duration, note.Duration); Assert.AreEqual(clonedNote.StartIndex, note.StartIndex); diff --git a/osu.Game.Rulesets.Karaoke/Objects/Note.cs b/osu.Game.Rulesets.Karaoke/Objects/Note.cs index f6876c1ef..dd9e2a2ce 100644 --- a/osu.Game.Rulesets.Karaoke/Objects/Note.cs +++ b/osu.Game.Rulesets.Karaoke/Objects/Note.cs @@ -3,6 +3,7 @@ using Newtonsoft.Json; using osu.Framework.Bindables; +using osu.Game.IO.Serialization; using osu.Game.Rulesets.Judgements; using osu.Game.Rulesets.Karaoke.Configuration; using osu.Game.Rulesets.Karaoke.Judgements; @@ -75,7 +76,6 @@ public virtual Tone Tone /// /// Duration /// - [JsonIgnore] public double Duration { get; set; } /// @@ -114,19 +114,11 @@ public Lyric ParentLyric public Note DeepClone() { - // (Note)MemberwiseClone(); + string serializeString = this.Serialize(); + var note = serializeString.Deserialize(); + note.ParentLyric = ParentLyric; - return new() - { - Text = Text, - RubyText = RubyText, - Display = Display, - Tone = Tone, - Duration = Duration, - StartIndex = StartIndex, - EndIndex = EndIndex, - ParentLyric = ParentLyric - }; + return note; } } } From 8b13605e65d284aece34276bbebb22486e194b3f Mon Sep 17 00:00:00 2001 From: andy840119 Date: Sat, 30 Jul 2022 17:47:41 +0800 Subject: [PATCH 3/6] Implement the deep clone the lyric and add the test case for it. --- .../Objects/LyricTest.cs | 84 +++++++++++++++++++ osu.Game.Rulesets.Karaoke/Objects/Lyric.cs | 17 +++- 2 files changed, 100 insertions(+), 1 deletion(-) create mode 100644 osu.Game.Rulesets.Karaoke.Tests/Objects/LyricTest.cs diff --git a/osu.Game.Rulesets.Karaoke.Tests/Objects/LyricTest.cs b/osu.Game.Rulesets.Karaoke.Tests/Objects/LyricTest.cs new file mode 100644 index 000000000..eb629e24a --- /dev/null +++ b/osu.Game.Rulesets.Karaoke.Tests/Objects/LyricTest.cs @@ -0,0 +1,84 @@ +// Copyright (c) andy840119 . Licensed under the GPL Licence. +// See the LICENCE file in the repository root for full licence text. + +using System.Collections.Generic; +using System.Globalization; +using NUnit.Framework; +using osu.Game.Rulesets.Karaoke.Objects; +using osu.Game.Rulesets.Karaoke.Objects.Types; +using osu.Game.Rulesets.Karaoke.Tests.Asserts; +using osu.Game.Rulesets.Karaoke.Tests.Helper; + +namespace osu.Game.Rulesets.Karaoke.Tests.Objects +{ + public class LyricTest + { + [TestCase] + public void TestClone() + { + var referencedLyric = new Lyric(); + + var lyric = new Lyric + { + ID = 1, + Text = "カラオケ", + TimeTags = TestCaseTagHelper.ParseTimeTags(new[] { "[0,start]:1000", "[1,start]:2000", "[2,start]:3000", "[3,start]:4000", "[3,end]:5000" }), + RubyTags = TestCaseTagHelper.ParseRubyTags(new[] { "[0,1]:か", "[1,2]:ら", "[2,3]:お", "[3,4]:け" }), + RomajiTags = TestCaseTagHelper.ParseRomajiTags(new[] { "[0,2]:ka", "[2,4]:ra", "[4,5]:o", "[5,7]:ke" }), + StartTime = 1000, + Duration = 4000, + Singers = new[] { 1, 2 }, + Translates = new Dictionary + { + { new CultureInfo("en-US"), "karaoke" } + }, + Language = new CultureInfo("ja-JP"), + Order = 1, + Lock = LockState.None, + ReferenceLyric = referencedLyric + }; + + var clonedLyric = lyric.DeepClone(); + + Assert.AreEqual(clonedLyric.ID, lyric.ID); + + Assert.AreNotSame(clonedLyric.TextBindable, lyric.TextBindable); + Assert.AreEqual(clonedLyric.Text, lyric.Text); + + Assert.AreNotSame(clonedLyric.TimeTagsVersion, lyric.TimeTagsVersion); + Assert.AreNotSame(clonedLyric.TimeTagsBindable, lyric.TimeTagsBindable); + TimeTagAssert.ArePropertyEqual(clonedLyric.TimeTags, lyric.TimeTags); + + Assert.AreNotSame(clonedLyric.RubyTagsVersion, lyric.RubyTagsVersion); + Assert.AreNotSame(clonedLyric.RubyTagsBindable, lyric.RubyTagsBindable); + TextTagAssert.ArePropertyEqual(clonedLyric.RubyTags, lyric.RubyTags); + + Assert.AreNotSame(clonedLyric.RomajiTagsVersion, lyric.RomajiTagsVersion); + Assert.AreNotSame(clonedLyric.RomajiTagsBindable, lyric.RomajiTagsBindable); + TextTagAssert.ArePropertyEqual(clonedLyric.RomajiTags, lyric.RomajiTags); + + Assert.AreNotSame(clonedLyric.StartTimeBindable, lyric.StartTimeBindable); + Assert.AreEqual(clonedLyric.StartTime, lyric.StartTime); + + Assert.AreEqual(clonedLyric.Duration, lyric.Duration); + + Assert.AreNotSame(clonedLyric.SingersBindable, lyric.SingersBindable); + CollectionAssert.AreEquivalent(clonedLyric.Singers, lyric.Singers); + + Assert.AreNotSame(clonedLyric.TranslateTextBindable, lyric.TranslateTextBindable); + CollectionAssert.AreEquivalent(clonedLyric.Translates, lyric.Translates); + + Assert.AreNotSame(clonedLyric.LanguageBindable, lyric.LanguageBindable); + Assert.AreEqual(clonedLyric.Language, lyric.Language); + + Assert.AreNotSame(clonedLyric.OrderBindable, lyric.OrderBindable); + Assert.AreEqual(clonedLyric.Order, lyric.Order); + + Assert.AreNotSame(clonedLyric.LockBindable, lyric.LockBindable); + Assert.AreEqual(clonedLyric.Lock, lyric.Lock); + + Assert.AreNotSame(clonedLyric.ReferenceLyricBindable, lyric.ReferenceLyricBindable); + Assert.AreSame(clonedLyric.ReferenceLyric, lyric.ReferenceLyric); + } + } +} diff --git a/osu.Game.Rulesets.Karaoke/Objects/Lyric.cs b/osu.Game.Rulesets.Karaoke/Objects/Lyric.cs index 8a1fa4c8b..38904aa77 100644 --- a/osu.Game.Rulesets.Karaoke/Objects/Lyric.cs +++ b/osu.Game.Rulesets.Karaoke/Objects/Lyric.cs @@ -10,6 +10,7 @@ using osu.Game.Beatmaps; using osu.Game.Beatmaps.ControlPoints; using osu.Game.Extensions; +using osu.Game.IO.Serialization; using osu.Game.Rulesets.Judgements; using osu.Game.Rulesets.Karaoke.Beatmaps; using osu.Game.Rulesets.Karaoke.Judgements; @@ -18,10 +19,11 @@ using osu.Game.Rulesets.Karaoke.Utils; using osu.Game.Rulesets.Objects.Types; using osu.Game.Rulesets.Scoring; +using osu.Game.Utils; namespace osu.Game.Rulesets.Karaoke.Objects { - public class Lyric : KaraokeHitObject, IHasDuration, IHasSingers, IHasOrder, IHasLock, IHasPrimaryKey + public class Lyric : KaraokeHitObject, IHasDuration, IHasSingers, IHasOrder, IHasLock, IHasPrimaryKey, IDeepCloneable { /// /// Primary key. @@ -291,5 +293,18 @@ public void InitialWorkingTime() } protected override HitWindows CreateHitWindows() => new KaraokeLyricHitWindows(); + + public Lyric DeepClone() + { + string serializeString = this.Serialize(); + var lyric = serializeString.Deserialize(); + + // IDK why got the different value of `CultureInfo` if placing it as key of the dictionary. + // So make a copy in here. + lyric.Translates = Translates; + lyric.ReferenceLyric = ReferenceLyric; + + return lyric; + } } } From ab9ea5e1e3adf16ddf19babb5675c3d46a0accd6 Mon Sep 17 00:00:00 2001 From: andy840119 Date: Sat, 30 Jul 2022 22:00:52 +0800 Subject: [PATCH 4/6] Apply the karaoke specific json setting and convertor. Because it will have data-format issue when serialize the translates in the lyric. --- osu.Game.Rulesets.Karaoke/Objects/Lyric.cs | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/osu.Game.Rulesets.Karaoke/Objects/Lyric.cs b/osu.Game.Rulesets.Karaoke/Objects/Lyric.cs index 38904aa77..100413cd1 100644 --- a/osu.Game.Rulesets.Karaoke/Objects/Lyric.cs +++ b/osu.Game.Rulesets.Karaoke/Objects/Lyric.cs @@ -10,9 +10,9 @@ using osu.Game.Beatmaps; using osu.Game.Beatmaps.ControlPoints; using osu.Game.Extensions; -using osu.Game.IO.Serialization; using osu.Game.Rulesets.Judgements; using osu.Game.Rulesets.Karaoke.Beatmaps; +using osu.Game.Rulesets.Karaoke.IO.Serialization; using osu.Game.Rulesets.Karaoke.Judgements; using osu.Game.Rulesets.Karaoke.Objects.Types; using osu.Game.Rulesets.Karaoke.Scoring; @@ -296,12 +296,9 @@ public void InitialWorkingTime() public Lyric DeepClone() { - string serializeString = this.Serialize(); - var lyric = serializeString.Deserialize(); + string serializeString = JsonConvert.SerializeObject(this, KaraokeJsonSerializableExtensions.CreateGlobalSettings()); + var lyric = JsonConvert.DeserializeObject(serializeString, KaraokeJsonSerializableExtensions.CreateGlobalSettings())!; - // IDK why got the different value of `CultureInfo` if placing it as key of the dictionary. - // So make a copy in here. - lyric.Translates = Translates; lyric.ReferenceLyric = ReferenceLyric; return lyric; From 58086c3476462f32af36cb7df424d8b8ad2d5abb Mon Sep 17 00:00:00 2001 From: andy840119 Date: Sat, 30 Jul 2022 17:54:08 +0800 Subject: [PATCH 5/6] Use deep clone instead of copy the property in the utils. --- .../Utils/LyricsUtils.cs | 34 +++++++------------ 1 file changed, 13 insertions(+), 21 deletions(-) diff --git a/osu.Game.Rulesets.Karaoke/Utils/LyricsUtils.cs b/osu.Game.Rulesets.Karaoke/Utils/LyricsUtils.cs index 545feb0b2..b3211bd34 100644 --- a/osu.Game.Rulesets.Karaoke/Utils/LyricsUtils.cs +++ b/osu.Game.Rulesets.Karaoke/Utils/LyricsUtils.cs @@ -60,28 +60,20 @@ public static Tuple SplitLyric(Lyric lyric, int splitIndex) } } - var firstLyric = new Lyric - { - Text = lyric.Text[..splitIndex], - TimeTags = firstTimeTag.ToArray(), - RubyTags = lyric.RubyTags.Where(x => x.StartIndex < splitIndex && x.EndIndex <= splitIndex).ToArray(), - RomajiTags = lyric.RomajiTags.Where(x => x.StartIndex < splitIndex && x.EndIndex <= splitIndex).ToArray(), - // todo : should implement time and duration - Singers = lyric.Singers, - Language = lyric.Language, - }; - + // todo : should implement time and duration + var firstLyric = lyric.DeepClone(); + firstLyric.Text = lyric.Text[..splitIndex]; + firstLyric.TimeTags = firstTimeTag.ToArray(); + firstLyric.RubyTags = lyric.RubyTags.Where(x => x.StartIndex < splitIndex && x.EndIndex <= splitIndex).ToArray(); + firstLyric.RomajiTags = lyric.RomajiTags.Where(x => x.StartIndex < splitIndex && x.EndIndex <= splitIndex).ToArray(); + + // todo : should implement time and duration string secondLyricText = lyric.Text[splitIndex..]; - var secondLyric = new Lyric - { - Text = secondLyricText, - TimeTags = shiftingTimeTag(secondTimeTag.ToArray(), -splitIndex), - RubyTags = shiftingTextTag(lyric.RubyTags.Where(x => x.StartIndex >= splitIndex && x.EndIndex > splitIndex).ToArray(), secondLyricText, -splitIndex), - RomajiTags = shiftingTextTag(lyric.RomajiTags.Where(x => x.StartIndex >= splitIndex && x.EndIndex > splitIndex).ToArray(), secondLyricText, -splitIndex), - // todo : should implement time and duration - Singers = lyric.Singers, - Language = lyric.Language, - }; + var secondLyric = lyric.DeepClone(); + secondLyric.Text = secondLyricText; + secondLyric.TimeTags = shiftingTimeTag(secondTimeTag.ToArray(), -splitIndex); + secondLyric.RubyTags = shiftingTextTag(lyric.RubyTags.Where(x => x.StartIndex >= splitIndex && x.EndIndex > splitIndex).ToArray(), secondLyricText, -splitIndex); + secondLyric.RomajiTags = shiftingTextTag(lyric.RomajiTags.Where(x => x.StartIndex >= splitIndex && x.EndIndex > splitIndex).ToArray(), secondLyricText, -splitIndex); return new Tuple(firstLyric, secondLyric); } From d6dca609792e61d58bac9f4ef10f5407f6b99cf6 Mon Sep 17 00:00:00 2001 From: andy840119 Date: Sat, 30 Jul 2022 17:58:07 +0800 Subject: [PATCH 6/6] Fix the test case. Combine notes should combine the ruby text also. --- .../Editor/ChangeHandlers/Notes/NotesChangeHandlerTest.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Rulesets.Karaoke.Tests/Editor/ChangeHandlers/Notes/NotesChangeHandlerTest.cs b/osu.Game.Rulesets.Karaoke.Tests/Editor/ChangeHandlers/Notes/NotesChangeHandlerTest.cs index 291f955a3..101ce8015 100644 --- a/osu.Game.Rulesets.Karaoke.Tests/Editor/ChangeHandlers/Notes/NotesChangeHandlerTest.cs +++ b/osu.Game.Rulesets.Karaoke.Tests/Editor/ChangeHandlers/Notes/NotesChangeHandlerTest.cs @@ -76,7 +76,7 @@ public void TestCombine() var combinedNote = actualNotes.First(); Assert.AreEqual("カラ", combinedNote.Text); - Assert.AreEqual(null, combinedNote.RubyText); + Assert.AreEqual("から", combinedNote.RubyText); Assert.AreEqual(1000, combinedNote.StartTime); Assert.AreEqual(1000, combinedNote.Duration); });