From 52a243afcabde3ccc375aaf6e79a9f50c046f54b Mon Sep 17 00:00:00 2001 From: Endrik Tombak Date: Wed, 25 Aug 2021 21:42:15 +0300 Subject: [PATCH 01/21] Don't select random beatmap when previous was filtered --- .../SongSelect/TestSceneBeatmapCarousel.cs | 99 +++++++++++-------- osu.Game/Screens/Select/BeatmapCarousel.cs | 2 +- .../Carousel/CarouselGroupEagerSelect.cs | 25 ++++- 3 files changed, 82 insertions(+), 44 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs index 78ddfa9ed2..f42ff3811a 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs @@ -616,55 +616,72 @@ namespace osu.Game.Tests.Visual.SongSelect AddAssert("Selection was remembered", () => eagerSelectedIDs.Count == 1); } - [Test] - public void TestRandomFallbackOnNonMatchingPrevious() + [TestCase(10, 1)] + [TestCase(10, 10)] + public void TestCarouselSelectsNextWhenPreviousIsFiltered(int makeThisManyGroups, int haveThisManySetsInGroup) { - List manySets = new List(); + List sets = new List(); - AddStep("populate maps", () => + for (int i = 0; i < makeThisManyGroups; i++) { - for (int i = 0; i < 10; i++) + for (int j = 0; j < haveThisManySetsInGroup; j++) { - var set = createTestBeatmapSet(i); - - foreach (var b in set.Beatmaps) + var testBeatmap = createTestBeatmapSet(i * haveThisManySetsInGroup + j + 1); + var rulesetID = i % 3; + testBeatmap.Beatmaps.ForEach(b => { - // all taiko except for first - int ruleset = i > 0 ? 1 : 0; - - b.Ruleset = rulesets.GetRuleset(ruleset); - b.RulesetID = ruleset; - } - - manySets.Add(set); + b.Ruleset = rulesets.AvailableRulesets.ElementAt(rulesetID); + b.RulesetID = rulesetID; + }); + sets.Add(testBeatmap); } - }); - - loadBeatmaps(manySets); - - for (int i = 0; i < 10; i++) - { - AddStep("Reset filter", () => carousel.Filter(new FilterCriteria(), false)); - - AddStep("select first beatmap", () => carousel.SelectBeatmap(manySets.First().Beatmaps.First())); - - AddStep("Toggle non-matching filter", () => - { - carousel.Filter(new FilterCriteria { SearchText = Guid.NewGuid().ToString() }, false); - }); - - AddAssert("selection lost", () => carousel.SelectedBeatmap == null); - - AddStep("Restore different ruleset filter", () => - { - carousel.Filter(new FilterCriteria { Ruleset = rulesets.GetRuleset(1) }, false); - eagerSelectedIDs.Add(carousel.SelectedBeatmapSet.ID); - }); - - AddAssert("selection changed", () => carousel.SelectedBeatmap != manySets.First().Beatmaps.First()); } - AddAssert("Selection was random", () => eagerSelectedIDs.Count > 2); + loadBeatmaps(sets); + setSelected(1, 1); + advanceSelection(false); + + for (int i = 1; i < makeThisManyGroups; i++) + { + var rulesetID = i % 3; + AddStep($"Toggle filter to ruleset {rulesetID}", () => + { + carousel.Filter(new FilterCriteria { Ruleset = rulesets.AvailableRulesets.ElementAt(rulesetID) }, false); + carousel.Filter(new FilterCriteria(), false); + }); + waitForSelection(i * haveThisManySetsInGroup + 1); + } + } + + [Test] + public void TestCarouselSelectsBackwardsWhenPreviousIsFilteredNearTheEnd() + { + List sets = new List(); + + for (int i = 1; i <= 40; i++) + { + var testBeatmap = createTestBeatmapSet(i); + var rulesetID = (i - 1) / 10; + testBeatmap.Beatmaps.ForEach(b => + { + b.Ruleset = rulesets.AvailableRulesets.ElementAt(rulesetID); + b.RulesetID = rulesetID; + }); + sets.Add(testBeatmap); + } + + loadBeatmaps(sets); + + for (int i = 1; i < 4; i++) + { + setSelected(i * 10 + 1, 1); + AddStep("Toggle filter to ruleset 0", () => + { + carousel.Filter(new FilterCriteria { Ruleset = rulesets.AvailableRulesets.ElementAt(0) }, false); + carousel.Filter(new FilterCriteria(), false); + }); + waitForSelection(10); + } } [Test] diff --git a/osu.Game/Screens/Select/BeatmapCarousel.cs b/osu.Game/Screens/Select/BeatmapCarousel.cs index b05b7aeb32..7c1403da82 100644 --- a/osu.Game/Screens/Select/BeatmapCarousel.cs +++ b/osu.Game/Screens/Select/BeatmapCarousel.cs @@ -911,7 +911,7 @@ namespace osu.Game.Screens.Select protected override void PerformSelection() { - if (LastSelected == null || LastSelected.Filtered.Value) + if (LastSelected == null) carousel?.SelectNextRandom(); else base.PerformSelection(); diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs index 9e8aad4b6f..79bcdbf1c1 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs @@ -101,8 +101,29 @@ namespace osu.Game.Screens.Select.Carousel protected virtual CarouselItem GetNextToSelect() { - return Children.Skip(lastSelectedIndex).FirstOrDefault(i => !i.Filtered.Value) ?? - Children.Reverse().Skip(InternalChildren.Count - lastSelectedIndex).FirstOrDefault(i => !i.Filtered.Value); + // our return value should be the item (that is not filtered and) that is nearest to the previously selected one + + // find nearest such item going forwards in selection + int forwardsDistance = 0; + var forwards = Children.Skip(lastSelectedIndex).SkipWhile((c) => + { + forwardsDistance++; + return c.Filtered.Value; + }).FirstOrDefault(); + // and backwards + int backwardsDistance = 0; + var backwards = Children.Reverse().Skip(InternalChildren.Count - lastSelectedIndex - 1).SkipWhile((c) => + { + backwardsDistance++; + return c.Filtered.Value; + }).FirstOrDefault(); + + // if only one direction had such an item, return that + if (forwards == null || backwards == null) + return forwards ?? backwards; + + // else return the closest item + return forwardsDistance < backwardsDistance ? forwards : backwards; } protected virtual void PerformSelection() From b484bd1af3ab2e49387d4eb9bb6f09b86b035cdf Mon Sep 17 00:00:00 2001 From: Endrik Tombak Date: Wed, 25 Aug 2021 22:43:58 +0300 Subject: [PATCH 02/21] Rename parameter and remove redundant parentheses --- .../Screens/Select/Carousel/CarouselGroupEagerSelect.cs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs index 79bcdbf1c1..2894209206 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs @@ -105,17 +105,17 @@ namespace osu.Game.Screens.Select.Carousel // find nearest such item going forwards in selection int forwardsDistance = 0; - var forwards = Children.Skip(lastSelectedIndex).SkipWhile((c) => + var forwards = Children.Skip(lastSelectedIndex).SkipWhile(item => { forwardsDistance++; - return c.Filtered.Value; + return item.Filtered.Value; }).FirstOrDefault(); // and backwards int backwardsDistance = 0; - var backwards = Children.Reverse().Skip(InternalChildren.Count - lastSelectedIndex - 1).SkipWhile((c) => + var backwards = Children.Reverse().Skip(InternalChildren.Count - lastSelectedIndex - 1).SkipWhile(item => { backwardsDistance++; - return c.Filtered.Value; + return item.Filtered.Value; }).FirstOrDefault(); // if only one direction had such an item, return that From a8d869c31cace6e8b47bda6f972459b07af50119 Mon Sep 17 00:00:00 2001 From: Endrik Tombak Date: Thu, 26 Aug 2021 18:31:19 +0300 Subject: [PATCH 03/21] Replace `SkipWhile()` LINQ with a while loop --- .../Carousel/CarouselGroupEagerSelect.cs | 39 ++++++++++--------- 1 file changed, 20 insertions(+), 19 deletions(-) diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs index 2894209206..aedeac04d3 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs @@ -99,31 +99,32 @@ namespace osu.Game.Screens.Select.Carousel PerformSelection(); } + /// + /// Finds the item this group would select next if it attempted selection + /// + /// An unfiltered item nearest to the last selected one or null if all items are filtered protected virtual CarouselItem GetNextToSelect() { - // our return value should be the item (that is not filtered and) that is nearest to the previously selected one + int forwardsIndex = lastSelectedIndex; + int backwardsIndex = lastSelectedIndex; - // find nearest such item going forwards in selection - int forwardsDistance = 0; - var forwards = Children.Skip(lastSelectedIndex).SkipWhile(item => + while (true) { - forwardsDistance++; - return item.Filtered.Value; - }).FirstOrDefault(); - // and backwards - int backwardsDistance = 0; - var backwards = Children.Reverse().Skip(InternalChildren.Count - lastSelectedIndex - 1).SkipWhile(item => - { - backwardsDistance++; - return item.Filtered.Value; - }).FirstOrDefault(); + if (forwardsIndex >= Children.Count) + return Children.Reverse().Skip(Children.Count - backwardsIndex).FirstOrDefault(item => !item.Filtered.Value); - // if only one direction had such an item, return that - if (forwards == null || backwards == null) - return forwards ?? backwards; + if (backwardsIndex < 0) + return Children.Skip(forwardsIndex).FirstOrDefault(item => !item.Filtered.Value); - // else return the closest item - return forwardsDistance < backwardsDistance ? forwards : backwards; + if (!Children[forwardsIndex].Filtered.Value) + return Children[forwardsIndex]; + + if (!Children[backwardsIndex].Filtered.Value) + return Children[backwardsIndex]; + + forwardsIndex++; + backwardsIndex--; + } } protected virtual void PerformSelection() From 8bd7837bf747600e67ffdc8f06062b847e10cd20 Mon Sep 17 00:00:00 2001 From: Endrik Tombak Date: Thu, 26 Aug 2021 19:29:58 +0300 Subject: [PATCH 04/21] Fix off-by-one skip amount --- osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs index aedeac04d3..9793b69eca 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs @@ -111,7 +111,7 @@ namespace osu.Game.Screens.Select.Carousel while (true) { if (forwardsIndex >= Children.Count) - return Children.Reverse().Skip(Children.Count - backwardsIndex).FirstOrDefault(item => !item.Filtered.Value); + return Children.Reverse().Skip(Children.Count - backwardsIndex - 1).FirstOrDefault(item => !item.Filtered.Value); if (backwardsIndex < 0) return Children.Skip(forwardsIndex).FirstOrDefault(item => !item.Filtered.Value); From 43e8e3e9229cef1d5ac72203d8e71c0f7584c49f Mon Sep 17 00:00:00 2001 From: Endrik Tombak Date: Fri, 27 Aug 2021 18:16:53 +0300 Subject: [PATCH 05/21] Add comments to `GetNextToSelect()` --- osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs index 9793b69eca..cdd6d48e66 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs @@ -110,18 +110,19 @@ namespace osu.Game.Screens.Select.Carousel while (true) { + // check if a direction has been exhausted and an item (or null) from the other direction should be returned if (forwardsIndex >= Children.Count) return Children.Reverse().Skip(Children.Count - backwardsIndex - 1).FirstOrDefault(item => !item.Filtered.Value); - if (backwardsIndex < 0) return Children.Skip(forwardsIndex).FirstOrDefault(item => !item.Filtered.Value); + // check if an unfiltered item has been reached if (!Children[forwardsIndex].Filtered.Value) return Children[forwardsIndex]; - if (!Children[backwardsIndex].Filtered.Value) return Children[backwardsIndex]; + // increment the indices forwardsIndex++; backwardsIndex--; } From 217fa00a72af23ae2971dba4d10beb9a5a185930 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Fri, 26 Aug 2022 19:32:49 +0900 Subject: [PATCH 06/21] Bring logic up-to-date --- .../SongSelect/TestSceneBeatmapCarousel.cs | 22 +++---------------- .../Carousel/CarouselGroupEagerSelect.cs | 14 ++++++------ 2 files changed, 10 insertions(+), 26 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs index 8f7d630a19..7623704a4b 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs @@ -834,16 +834,7 @@ namespace osu.Game.Tests.Visual.SongSelect for (int i = 0; i < makeThisManyGroups; i++) { for (int j = 0; j < haveThisManySetsInGroup; j++) - { - var testBeatmap = createTestBeatmapSet(i * haveThisManySetsInGroup + j + 1); - var rulesetID = i % 3; - testBeatmap.Beatmaps.ForEach(b => - { - b.Ruleset = rulesets.AvailableRulesets.ElementAt(rulesetID); - b.RulesetID = rulesetID; - }); - sets.Add(testBeatmap); - } + sets.Add(TestResources.CreateTestBeatmapSetInfo(i * haveThisManySetsInGroup + j + 1)); } loadBeatmaps(sets); @@ -852,7 +843,7 @@ namespace osu.Game.Tests.Visual.SongSelect for (int i = 1; i < makeThisManyGroups; i++) { - var rulesetID = i % 3; + int rulesetID = i % 3; AddStep($"Toggle filter to ruleset {rulesetID}", () => { carousel.Filter(new FilterCriteria { Ruleset = rulesets.AvailableRulesets.ElementAt(rulesetID) }, false); @@ -869,14 +860,7 @@ namespace osu.Game.Tests.Visual.SongSelect for (int i = 1; i <= 40; i++) { - var testBeatmap = createTestBeatmapSet(i); - var rulesetID = (i - 1) / 10; - testBeatmap.Beatmaps.ForEach(b => - { - b.Ruleset = rulesets.AvailableRulesets.ElementAt(rulesetID); - b.RulesetID = rulesetID; - }); - sets.Add(testBeatmap); + sets.Add(TestResources.CreateTestBeatmapSetInfo(i)); } loadBeatmaps(sets); diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs index 432f795f4c..2c1ccebfab 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs @@ -120,16 +120,16 @@ namespace osu.Game.Screens.Select.Carousel while (true) { // check if a direction has been exhausted and an item (or null) from the other direction should be returned - if (forwardsIndex >= Children.Count) - return Children.Reverse().Skip(Children.Count - backwardsIndex - 1).FirstOrDefault(item => !item.Filtered.Value); + if (forwardsIndex >= Items.Count) + return Items.Reverse().Skip(Items.Count - backwardsIndex - 1).FirstOrDefault(item => !item.Filtered.Value); if (backwardsIndex < 0) - return Children.Skip(forwardsIndex).FirstOrDefault(item => !item.Filtered.Value); + return Items.Skip(forwardsIndex).FirstOrDefault(item => !item.Filtered.Value); // check if an unfiltered item has been reached - if (!Children[forwardsIndex].Filtered.Value) - return Children[forwardsIndex]; - if (!Children[backwardsIndex].Filtered.Value) - return Children[backwardsIndex]; + if (!Items[forwardsIndex].Filtered.Value) + return Items[forwardsIndex]; + if (!Items[backwardsIndex].Filtered.Value) + return Items[backwardsIndex]; // increment the indices forwardsIndex++; From 9def12c60bceb6c75a92f74be14aafd1d10b8fe6 Mon Sep 17 00:00:00 2001 From: Endrik Tombak Date: Mon, 24 Oct 2022 22:50:56 +0300 Subject: [PATCH 07/21] Write new tests --- .../SongSelect/TestSceneBeatmapCarousel.cs | 103 ++++++++++-------- 1 file changed, 57 insertions(+), 46 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs index 0e72463d1e..303af61d20 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs @@ -827,6 +827,63 @@ namespace osu.Game.Tests.Visual.SongSelect AddStep("remove single ruleset set", () => carousel.RemoveBeatmapSet(testSingle)); } + [Test] + public void TestCarouselSelectsNextWhenPreviousIsFiltered() + { + List sets = new List(); + + // 10 sets that go osu! -> taiko -> catch -> osu! -> ... + for (int i = 0; i < 10; i++) + { + var rulesetInfo = rulesets.AvailableRulesets.ElementAt(i % 3); + sets.Add(TestResources.CreateTestBeatmapSetInfo(5, new[] { rulesetInfo })); + } + + // Sort mode is important to keep the ruleset order + loadBeatmaps(sets, () => new FilterCriteria { Sort = SortMode.Title }); + setSelected(1, 1); + + for (int i = 1; i < 10; i++) + { + var rulesetInfo = rulesets.AvailableRulesets.ElementAt(i % 3); + AddStep($"Set ruleset to {rulesetInfo.ShortName}", () => + { + carousel.Filter(new FilterCriteria { Ruleset = rulesetInfo, Sort = SortMode.Title }, false); + }); + waitForSelection(i + 1, 1); + } + } + + [Test] + public void TestCarouselSelectsBackwardsWhenDistanceIsShorter() + { + List sets = new List(); + + // 10 sets that go taiko, osu!, osu!, osu!, taiko, osu!, osu!, osu!, ... + for (int i = 0; i < 10; i++) + { + var rulesetInfo = rulesets.AvailableRulesets.ElementAt(i % 4 == 0 ? 1 : 0); + sets.Add(TestResources.CreateTestBeatmapSetInfo(5, new[] { rulesetInfo })); + } + + // Sort mode is important to keep the ruleset order + loadBeatmaps(sets, () => new FilterCriteria { Sort = SortMode.Title }); + + for (int i = 2; i < 10; i += 4) + { + setSelected(i, 1); + AddStep("Set ruleset to taiko", () => + { + carousel.Filter(new FilterCriteria { Ruleset = rulesets.AvailableRulesets.ElementAt(1), Sort = SortMode.Title }, false); + }); + waitForSelection(i - 1, 1); + AddStep("Remove ruleset filter", () => + { + carousel.Filter(new FilterCriteria { Sort = SortMode.Title }, false); + }); + } + } + [Test] public void TestCarouselRemembersSelection() { @@ -862,52 +919,6 @@ namespace osu.Game.Tests.Visual.SongSelect AddAssert("Selection was remembered", () => eagerSelectedIDs.Count == 1); } - [Test] - public void TestRandomFallbackOnNonMatchingPrevious() - { - List manySets = new List(); - - AddStep("populate maps", () => - { - manySets.Clear(); - - for (int i = 0; i < 10; i++) - { - manySets.Add(TestResources.CreateTestBeatmapSetInfo(3, new[] - { - // all taiko except for first - rulesets.GetRuleset(i > 0 ? 1 : 0) - })); - } - }); - - loadBeatmaps(manySets); - - for (int i = 0; i < 10; i++) - { - AddStep("Reset filter", () => carousel.Filter(new FilterCriteria(), false)); - - AddStep("select first beatmap", () => carousel.SelectBeatmap(manySets.First().Beatmaps.First())); - - AddStep("Toggle non-matching filter", () => - { - carousel.Filter(new FilterCriteria { SearchText = Guid.NewGuid().ToString() }, false); - }); - - AddAssert("selection lost", () => carousel.SelectedBeatmapInfo == null); - - AddStep("Restore different ruleset filter", () => - { - carousel.Filter(new FilterCriteria { Ruleset = rulesets.GetRuleset(1) }, false); - eagerSelectedIDs.Add(carousel.SelectedBeatmapSet!.ID); - }); - - AddAssert("selection changed", () => !carousel.SelectedBeatmapInfo!.Equals(manySets.First().Beatmaps.First())); - } - - AddAssert("Selection was random", () => eagerSelectedIDs.Count > 2); - } - [Test] public void TestFilteringByUserStarDifficulty() { From b95091fbb0a79bbb4d42a173c30354aabfb864a0 Mon Sep 17 00:00:00 2001 From: Endrik Tombak Date: Mon, 24 Oct 2022 22:55:09 +0300 Subject: [PATCH 08/21] Move test to improve diff readability --- .../SongSelect/TestSceneBeatmapCarousel.cs | 114 +++++++++--------- 1 file changed, 57 insertions(+), 57 deletions(-) diff --git a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs index 303af61d20..eacaf7f92e 100644 --- a/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs +++ b/osu.Game.Tests/Visual/SongSelect/TestSceneBeatmapCarousel.cs @@ -827,63 +827,6 @@ namespace osu.Game.Tests.Visual.SongSelect AddStep("remove single ruleset set", () => carousel.RemoveBeatmapSet(testSingle)); } - [Test] - public void TestCarouselSelectsNextWhenPreviousIsFiltered() - { - List sets = new List(); - - // 10 sets that go osu! -> taiko -> catch -> osu! -> ... - for (int i = 0; i < 10; i++) - { - var rulesetInfo = rulesets.AvailableRulesets.ElementAt(i % 3); - sets.Add(TestResources.CreateTestBeatmapSetInfo(5, new[] { rulesetInfo })); - } - - // Sort mode is important to keep the ruleset order - loadBeatmaps(sets, () => new FilterCriteria { Sort = SortMode.Title }); - setSelected(1, 1); - - for (int i = 1; i < 10; i++) - { - var rulesetInfo = rulesets.AvailableRulesets.ElementAt(i % 3); - AddStep($"Set ruleset to {rulesetInfo.ShortName}", () => - { - carousel.Filter(new FilterCriteria { Ruleset = rulesetInfo, Sort = SortMode.Title }, false); - }); - waitForSelection(i + 1, 1); - } - } - - [Test] - public void TestCarouselSelectsBackwardsWhenDistanceIsShorter() - { - List sets = new List(); - - // 10 sets that go taiko, osu!, osu!, osu!, taiko, osu!, osu!, osu!, ... - for (int i = 0; i < 10; i++) - { - var rulesetInfo = rulesets.AvailableRulesets.ElementAt(i % 4 == 0 ? 1 : 0); - sets.Add(TestResources.CreateTestBeatmapSetInfo(5, new[] { rulesetInfo })); - } - - // Sort mode is important to keep the ruleset order - loadBeatmaps(sets, () => new FilterCriteria { Sort = SortMode.Title }); - - for (int i = 2; i < 10; i += 4) - { - setSelected(i, 1); - AddStep("Set ruleset to taiko", () => - { - carousel.Filter(new FilterCriteria { Ruleset = rulesets.AvailableRulesets.ElementAt(1), Sort = SortMode.Title }, false); - }); - waitForSelection(i - 1, 1); - AddStep("Remove ruleset filter", () => - { - carousel.Filter(new FilterCriteria { Sort = SortMode.Title }, false); - }); - } - } - [Test] public void TestCarouselRemembersSelection() { @@ -966,6 +909,63 @@ namespace osu.Game.Tests.Visual.SongSelect checkVisibleItemCount(true, 15); } + [Test] + public void TestCarouselSelectsNextWhenPreviousIsFiltered() + { + List sets = new List(); + + // 10 sets that go osu! -> taiko -> catch -> osu! -> ... + for (int i = 0; i < 10; i++) + { + var rulesetInfo = rulesets.AvailableRulesets.ElementAt(i % 3); + sets.Add(TestResources.CreateTestBeatmapSetInfo(5, new[] { rulesetInfo })); + } + + // Sort mode is important to keep the ruleset order + loadBeatmaps(sets, () => new FilterCriteria { Sort = SortMode.Title }); + setSelected(1, 1); + + for (int i = 1; i < 10; i++) + { + var rulesetInfo = rulesets.AvailableRulesets.ElementAt(i % 3); + AddStep($"Set ruleset to {rulesetInfo.ShortName}", () => + { + carousel.Filter(new FilterCriteria { Ruleset = rulesetInfo, Sort = SortMode.Title }, false); + }); + waitForSelection(i + 1, 1); + } + } + + [Test] + public void TestCarouselSelectsBackwardsWhenDistanceIsShorter() + { + List sets = new List(); + + // 10 sets that go taiko, osu!, osu!, osu!, taiko, osu!, osu!, osu!, ... + for (int i = 0; i < 10; i++) + { + var rulesetInfo = rulesets.AvailableRulesets.ElementAt(i % 4 == 0 ? 1 : 0); + sets.Add(TestResources.CreateTestBeatmapSetInfo(5, new[] { rulesetInfo })); + } + + // Sort mode is important to keep the ruleset order + loadBeatmaps(sets, () => new FilterCriteria { Sort = SortMode.Title }); + + for (int i = 2; i < 10; i += 4) + { + setSelected(i, 1); + AddStep("Set ruleset to taiko", () => + { + carousel.Filter(new FilterCriteria { Ruleset = rulesets.AvailableRulesets.ElementAt(1), Sort = SortMode.Title }, false); + }); + waitForSelection(i - 1, 1); + AddStep("Remove ruleset filter", () => + { + carousel.Filter(new FilterCriteria { Sort = SortMode.Title }, false); + }); + } + } + private void loadBeatmaps(List beatmapSets = null, Func initialCriteria = null, Action carouselAdjust = null, int? count = null, bool randomDifficulties = false) { From 2456a18bc1be49d23b3beb0e1ca2fed087f148d2 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 25 Oct 2022 12:48:17 +0900 Subject: [PATCH 09/21] Rewrite implementation to avoid duplicated LINQ returns --- .../Carousel/CarouselGroupEagerSelect.cs | 20 +++++++++---------- 1 file changed, 9 insertions(+), 11 deletions(-) diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs index 2c1ccebfab..a96d768ff9 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs @@ -115,26 +115,24 @@ namespace osu.Game.Screens.Select.Carousel protected virtual CarouselItem GetNextToSelect() { int forwardsIndex = lastSelectedIndex; + bool hasForwards; + int backwardsIndex = lastSelectedIndex; + bool hasBackwards; - while (true) + while ((hasBackwards = backwardsIndex >= 0) | (hasForwards = forwardsIndex < Items.Count)) { - // check if a direction has been exhausted and an item (or null) from the other direction should be returned - if (forwardsIndex >= Items.Count) - return Items.Reverse().Skip(Items.Count - backwardsIndex - 1).FirstOrDefault(item => !item.Filtered.Value); - if (backwardsIndex < 0) - return Items.Skip(forwardsIndex).FirstOrDefault(item => !item.Filtered.Value); - - // check if an unfiltered item has been reached - if (!Items[forwardsIndex].Filtered.Value) + if (hasForwards && !Items[forwardsIndex].Filtered.Value) return Items[forwardsIndex]; - if (!Items[backwardsIndex].Filtered.Value) + + if (hasBackwards && !Items[backwardsIndex].Filtered.Value) return Items[backwardsIndex]; - // increment the indices forwardsIndex++; backwardsIndex--; } + + return null; } protected virtual void PerformSelection() From 22ea9a8ab4f2d4c07820935f3c3dc823c8c1b65b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 25 Oct 2022 13:34:21 +0900 Subject: [PATCH 10/21] Add early return of carousel group has no items --- osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs | 3 +++ 1 file changed, 3 insertions(+) diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs index a96d768ff9..6a53f8f527 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs @@ -114,6 +114,9 @@ namespace osu.Game.Screens.Select.Carousel /// An unfiltered item nearest to the last selected one or null if all items are filtered protected virtual CarouselItem GetNextToSelect() { + if (Items.Count == 0) + return null; + int forwardsIndex = lastSelectedIndex; bool hasForwards; From cf3bf41b4902f754d64344bda326928190dd3ff7 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Tue, 25 Oct 2022 18:57:15 +0900 Subject: [PATCH 11/21] Harden and simplify logic to avoid weird issues --- .../Select/Carousel/CarouselGroupEagerSelect.cs | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs index 6a53f8f527..34865c7f51 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs @@ -118,13 +118,16 @@ namespace osu.Game.Screens.Select.Carousel return null; int forwardsIndex = lastSelectedIndex; - bool hasForwards; - int backwardsIndex = lastSelectedIndex; - bool hasBackwards; - while ((hasBackwards = backwardsIndex >= 0) | (hasForwards = forwardsIndex < Items.Count)) + while (true) { + bool hasBackwards = backwardsIndex >= 0 && backwardsIndex < Items.Count; + bool hasForwards = forwardsIndex < Items.Count; + + if (!hasBackwards && !hasForwards) + return null; + if (hasForwards && !Items[forwardsIndex].Filtered.Value) return Items[forwardsIndex]; @@ -134,8 +137,6 @@ namespace osu.Game.Screens.Select.Carousel forwardsIndex++; backwardsIndex--; } - - return null; } protected virtual void PerformSelection() From 92ab39f5c2d94c34964b7c22894f3914c7aa8311 Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 25 Oct 2022 19:16:23 +0900 Subject: [PATCH 12/21] Fix hold note disappear instantly --- osu.Game.Rulesets.Mania/Objects/Drawables/DrawableHoldNote.cs | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/osu.Game.Rulesets.Mania/Objects/Drawables/DrawableHoldNote.cs b/osu.Game.Rulesets.Mania/Objects/Drawables/DrawableHoldNote.cs index 48647f9f5f..14dbc432ff 100644 --- a/osu.Game.Rulesets.Mania/Objects/Drawables/DrawableHoldNote.cs +++ b/osu.Game.Rulesets.Mania/Objects/Drawables/DrawableHoldNote.cs @@ -69,6 +69,8 @@ namespace osu.Game.Rulesets.Mania.Objects.Drawables /// private double? releaseTime; + public override double MaximumJudgementOffset => Tail.MaximumJudgementOffset; + public DrawableHoldNote() : this(null) { @@ -260,7 +262,7 @@ namespace osu.Game.Rulesets.Mania.Objects.Drawables tick.MissForcefully(); } - ApplyResult(r => r.Type = r.Judgement.MaxResult); + ApplyResult(r => r.Type = Tail.IsHit ? r.Judgement.MaxResult : r.Judgement.MinResult); endHold(); } From 0b8b3223222d4cfe3311e65707bd5fe3d59ad7d5 Mon Sep 17 00:00:00 2001 From: ekrctb Date: Tue, 25 Oct 2022 19:48:33 +0900 Subject: [PATCH 13/21] Fix tests --- osu.Game.Rulesets.Mania.Tests/TestSceneHoldNoteInput.cs | 2 +- osu.Game.Rulesets.Mania.Tests/TestSceneOutOfOrderHits.cs | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/osu.Game.Rulesets.Mania.Tests/TestSceneHoldNoteInput.cs b/osu.Game.Rulesets.Mania.Tests/TestSceneHoldNoteInput.cs index 8f776ff507..0296303867 100644 --- a/osu.Game.Rulesets.Mania.Tests/TestSceneHoldNoteInput.cs +++ b/osu.Game.Rulesets.Mania.Tests/TestSceneHoldNoteInput.cs @@ -49,7 +49,7 @@ namespace osu.Game.Rulesets.Mania.Tests assertHeadJudgement(HitResult.Miss); assertTickJudgement(HitResult.LargeTickMiss); assertTailJudgement(HitResult.Miss); - assertNoteJudgement(HitResult.IgnoreHit); + assertNoteJudgement(HitResult.IgnoreMiss); } /// diff --git a/osu.Game.Rulesets.Mania.Tests/TestSceneOutOfOrderHits.cs b/osu.Game.Rulesets.Mania.Tests/TestSceneOutOfOrderHits.cs index 1f139b5b78..464dbecee5 100644 --- a/osu.Game.Rulesets.Mania.Tests/TestSceneOutOfOrderHits.cs +++ b/osu.Game.Rulesets.Mania.Tests/TestSceneOutOfOrderHits.cs @@ -76,8 +76,8 @@ namespace osu.Game.Rulesets.Mania.Tests performTest(objects, new List()); - addJudgementAssert(objects[0], HitResult.IgnoreHit); - addJudgementAssert(objects[1], HitResult.IgnoreHit); + addJudgementAssert(objects[0], HitResult.IgnoreMiss); + addJudgementAssert(objects[1], HitResult.IgnoreMiss); } [Test] From 93e9b4a2c5329a46668b6f4261680abfb763a7c9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 25 Oct 2022 19:53:05 +0200 Subject: [PATCH 14/21] Add failing test case --- .../Mods/TestSceneTaikoModFlashlight.cs | 36 +++++++++++++++++++ .../Mods/TaikoModFlashlight.cs | 8 ++--- 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/osu.Game.Rulesets.Taiko.Tests/Mods/TestSceneTaikoModFlashlight.cs b/osu.Game.Rulesets.Taiko.Tests/Mods/TestSceneTaikoModFlashlight.cs index 417b59f5d2..d55ce17e6c 100644 --- a/osu.Game.Rulesets.Taiko.Tests/Mods/TestSceneTaikoModFlashlight.cs +++ b/osu.Game.Rulesets.Taiko.Tests/Mods/TestSceneTaikoModFlashlight.cs @@ -1,8 +1,12 @@ // Copyright (c) ppy Pty Ltd . Licensed under the MIT Licence. // See the LICENCE file in the repository root for full licence text. +using System.Linq; using NUnit.Framework; +using osu.Framework.Testing; using osu.Game.Rulesets.Taiko.Mods; +using osu.Game.Rulesets.Taiko.UI; +using osuTK; namespace osu.Game.Rulesets.Taiko.Tests.Mods { @@ -16,5 +20,37 @@ namespace osu.Game.Rulesets.Taiko.Tests.Mods [Test] public void TestComboBasedSize([Values] bool comboBasedSize) => CreateModTest(new ModTestData { Mod = new TaikoModFlashlight { ComboBasedSize = { Value = comboBasedSize } }, PassCondition = () => true }); + + [Test] + public void TestFlashlightAlwaysHasNonZeroSize() + { + bool failed = false; + + CreateModTest(new ModTestData + { + Mod = new TestTaikoModFlashlight { ComboBasedSize = { Value = true } }, + Autoplay = false, + PassCondition = () => + { + failed |= this.ChildrenOfType().SingleOrDefault()?.FlashlightSize.Y == 0; + return !failed; + } + }); + } + + private class TestTaikoModFlashlight : TaikoModFlashlight + { + protected override Flashlight CreateFlashlight() => new TestTaikoFlashlight(this, Playfield); + + public class TestTaikoFlashlight : TaikoFlashlight + { + public TestTaikoFlashlight(TaikoModFlashlight modFlashlight, TaikoPlayfield taikoPlayfield) + : base(modFlashlight, taikoPlayfield) + { + } + + public new Vector2 FlashlightSize => base.FlashlightSize; + } + } } } diff --git a/osu.Game.Rulesets.Taiko/Mods/TaikoModFlashlight.cs b/osu.Game.Rulesets.Taiko/Mods/TaikoModFlashlight.cs index 98f954ad29..d5b0b64036 100644 --- a/osu.Game.Rulesets.Taiko/Mods/TaikoModFlashlight.cs +++ b/osu.Game.Rulesets.Taiko/Mods/TaikoModFlashlight.cs @@ -27,17 +27,17 @@ namespace osu.Game.Rulesets.Taiko.Mods public override float DefaultFlashlightSize => 200; - protected override Flashlight CreateFlashlight() => new TaikoFlashlight(this, playfield); + protected override Flashlight CreateFlashlight() => new TaikoFlashlight(this, Playfield); - private TaikoPlayfield playfield = null!; + protected TaikoPlayfield Playfield { get; private set; } = null!; public override void ApplyToDrawableRuleset(DrawableRuleset drawableRuleset) { - playfield = (TaikoPlayfield)drawableRuleset.Playfield; + Playfield = (TaikoPlayfield)drawableRuleset.Playfield; base.ApplyToDrawableRuleset(drawableRuleset); } - private class TaikoFlashlight : Flashlight + public class TaikoFlashlight : Flashlight { private readonly LayoutValue flashlightProperties = new LayoutValue(Invalidation.RequiredParentSizeToFit | Invalidation.DrawInfo); private readonly TaikoPlayfield taikoPlayfield; From e3bc1126d75e204b08a85059b6f818c4c292d60d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 25 Oct 2022 20:03:22 +0200 Subject: [PATCH 15/21] Fix invalid flashlight size calculation This regressed in #20714. One usage of `getSizeFor(0)` was replaced by `adjustSize(GetSize())`, but another usage of `getSizeFor(Combo.Value)` was replaced by `adjustSize(Combo.Value)`, which is not correct, since `adjustSize()` is expecting to receive a combo-based size, rather than a combo value directly. --- osu.Game.Rulesets.Taiko/Mods/TaikoModFlashlight.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game.Rulesets.Taiko/Mods/TaikoModFlashlight.cs b/osu.Game.Rulesets.Taiko/Mods/TaikoModFlashlight.cs index d5b0b64036..2b3dc799ab 100644 --- a/osu.Game.Rulesets.Taiko/Mods/TaikoModFlashlight.cs +++ b/osu.Game.Rulesets.Taiko/Mods/TaikoModFlashlight.cs @@ -75,7 +75,7 @@ namespace osu.Game.Rulesets.Taiko.Mods FlashlightPosition = ToLocalSpace(taikoPlayfield.HitTarget.ScreenSpaceDrawQuad.Centre); ClearTransforms(targetMember: nameof(FlashlightSize)); - FlashlightSize = adjustSize(Combo.Value); + FlashlightSize = adjustSize(GetSize()); flashlightProperties.Validate(); } From 974f22dd97a4f79fc9709a5431c2f1ecae73549f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Tue, 25 Oct 2022 20:11:29 +0200 Subject: [PATCH 16/21] Attempt to make flashlight code slightly more legible --- .../Mods/TaikoModFlashlight.cs | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/osu.Game.Rulesets.Taiko/Mods/TaikoModFlashlight.cs b/osu.Game.Rulesets.Taiko/Mods/TaikoModFlashlight.cs index 2b3dc799ab..46569c2495 100644 --- a/osu.Game.Rulesets.Taiko/Mods/TaikoModFlashlight.cs +++ b/osu.Game.Rulesets.Taiko/Mods/TaikoModFlashlight.cs @@ -47,21 +47,28 @@ namespace osu.Game.Rulesets.Taiko.Mods { this.taikoPlayfield = taikoPlayfield; - FlashlightSize = adjustSize(GetSize()); + FlashlightSize = adjustSizeForPlayfieldAspectRatio(GetSize()); FlashlightSmoothness = 1.4f; AddLayout(flashlightProperties); } - private Vector2 adjustSize(float size) + /// + /// Returns the aspect ratio-adjusted size of the flashlight. + /// This ensures that the size of the flashlight remains independent of taiko-specific aspect ratio adjustments. + /// + /// + /// The size of the flashlight. + /// The value provided here should always come from . + /// + private Vector2 adjustSizeForPlayfieldAspectRatio(float size) { - // Preserve flashlight size through the playfield's aspect adjustment. return new Vector2(0, size * taikoPlayfield.DrawHeight / TaikoPlayfield.DEFAULT_HEIGHT); } protected override void UpdateFlashlightSize(float size) { - this.TransformTo(nameof(FlashlightSize), adjustSize(size), FLASHLIGHT_FADE_DURATION); + this.TransformTo(nameof(FlashlightSize), adjustSizeForPlayfieldAspectRatio(size), FLASHLIGHT_FADE_DURATION); } protected override string FragmentShader => "CircularFlashlight"; @@ -75,7 +82,7 @@ namespace osu.Game.Rulesets.Taiko.Mods FlashlightPosition = ToLocalSpace(taikoPlayfield.HitTarget.ScreenSpaceDrawQuad.Centre); ClearTransforms(targetMember: nameof(FlashlightSize)); - FlashlightSize = adjustSize(GetSize()); + FlashlightSize = adjustSizeForPlayfieldAspectRatio(GetSize()); flashlightProperties.Validate(); } From 8505848b1a0efaf3682a7b703b94f95f95d902c2 Mon Sep 17 00:00:00 2001 From: Joseph Madamba Date: Tue, 25 Oct 2022 18:06:49 -0700 Subject: [PATCH 17/21] Scroll beatmap listing to top when searching via tags/source --- osu.Game/Overlays/BeatmapListingOverlay.cs | 1 + 1 file changed, 1 insertion(+) diff --git a/osu.Game/Overlays/BeatmapListingOverlay.cs b/osu.Game/Overlays/BeatmapListingOverlay.cs index 2be328427b..c73936da8a 100644 --- a/osu.Game/Overlays/BeatmapListingOverlay.cs +++ b/osu.Game/Overlays/BeatmapListingOverlay.cs @@ -115,6 +115,7 @@ namespace osu.Game.Overlays { filterControl.Search(query); Show(); + ScrollFlow.ScrollToStart(); } protected override BeatmapListingHeader CreateHeader() => new BeatmapListingHeader(); From d9d083752aeac9d1342084f63d6495d5a0e78a8b Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 26 Oct 2022 11:03:11 +0900 Subject: [PATCH 18/21] Clamp backwards index to total current items to fix edge case of item removal --- osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs index 34865c7f51..6366fc8050 100644 --- a/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs +++ b/osu.Game/Screens/Select/Carousel/CarouselGroupEagerSelect.cs @@ -118,7 +118,7 @@ namespace osu.Game.Screens.Select.Carousel return null; int forwardsIndex = lastSelectedIndex; - int backwardsIndex = lastSelectedIndex; + int backwardsIndex = Math.Min(lastSelectedIndex, Items.Count - 1); while (true) { From 5f10b81af1dfaf0f717858961780538a1fc2498d Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 26 Oct 2022 12:55:59 +0900 Subject: [PATCH 19/21] Move distance snap grid test cursor behind grid to better visualise centre-points --- .../Editor/TestSceneOsuDistanceSnapGrid.cs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneOsuDistanceSnapGrid.cs b/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneOsuDistanceSnapGrid.cs index dc74d38cdc..f880739950 100644 --- a/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneOsuDistanceSnapGrid.cs +++ b/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneOsuDistanceSnapGrid.cs @@ -88,8 +88,8 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor RelativeSizeAxes = Axes.Both, Colour = Color4.SlateGray }, + new SnappingCursorContainer { GetSnapPosition = v => grid.GetSnappedPosition(grid.ToLocalSpace(v)).position }, grid = new OsuDistanceSnapGrid(new HitCircle { Position = grid_position }), - new SnappingCursorContainer { GetSnapPosition = v => grid.GetSnappedPosition(grid.ToLocalSpace(v)).position } }; }); @@ -166,8 +166,8 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor RelativeSizeAxes = Axes.Both, Colour = Color4.SlateGray }, + new SnappingCursorContainer { GetSnapPosition = v => grid.GetSnappedPosition(grid.ToLocalSpace(v)).position }, grid = new OsuDistanceSnapGrid(new HitCircle { Position = grid_position }, new HitCircle { StartTime = 200 }), - new SnappingCursorContainer { GetSnapPosition = v => grid.GetSnappedPosition(grid.ToLocalSpace(v)).position } }; }); From cab9a1c01203236748b6a72077dec351286d5d34 Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 26 Oct 2022 13:08:19 +0900 Subject: [PATCH 20/21] Add test coverage of distance snap grid failing visually --- .../Editor/TestSceneOsuDistanceSnapGrid.cs | 43 +++++++++++++++++-- 1 file changed, 40 insertions(+), 3 deletions(-) diff --git a/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneOsuDistanceSnapGrid.cs b/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneOsuDistanceSnapGrid.cs index f880739950..1b67fc2ca9 100644 --- a/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneOsuDistanceSnapGrid.cs +++ b/osu.Game.Rulesets.Osu.Tests/Editor/TestSceneOsuDistanceSnapGrid.cs @@ -4,12 +4,15 @@ #nullable disable using System; +using System.Linq; using NUnit.Framework; using osu.Framework.Allocation; using osu.Framework.Graphics; using osu.Framework.Graphics.Containers; using osu.Framework.Graphics.Shapes; +using osu.Framework.Graphics.UserInterface; using osu.Framework.Input; +using osu.Framework.Testing; using osu.Framework.Utils; using osu.Game.Beatmaps.ControlPoints; using osu.Game.Overlays; @@ -52,6 +55,7 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor }; private OsuDistanceSnapGrid grid; + private SnappingCursorContainer cursor; public TestSceneOsuDistanceSnapGrid() { @@ -88,7 +92,7 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor RelativeSizeAxes = Axes.Both, Colour = Color4.SlateGray }, - new SnappingCursorContainer { GetSnapPosition = v => grid.GetSnappedPosition(grid.ToLocalSpace(v)).position }, + cursor = new SnappingCursorContainer { GetSnapPosition = v => grid.GetSnappedPosition(grid.ToLocalSpace(v)).position }, grid = new OsuDistanceSnapGrid(new HitCircle { Position = grid_position }), }; }); @@ -154,6 +158,37 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor assertSnappedDistance(expectedDistance); } + [Test] + public void TestReferenceObjectNotOnSnapGrid() + { + AddStep("create grid", () => + { + Children = new Drawable[] + { + new Box + { + RelativeSizeAxes = Axes.Both, + Colour = Color4.SlateGray + }, + cursor = new SnappingCursorContainer { GetSnapPosition = v => grid.GetSnappedPosition(grid.ToLocalSpace(v)).position }, + grid = new OsuDistanceSnapGrid(new HitCircle + { + Position = grid_position, + // This is important. It sets the reference object to a point in time that isn't on the current snap divisor's grid. + // We are testing that the grid's display is offset correctly. + StartTime = 40, + }), + }; + }); + + AddStep("move mouse to point", () => InputManager.MoveMouseTo(grid.ToScreenSpace(grid_position + new Vector2(beat_length, 0) * 2))); + + AddAssert("Ensure cursor is on a grid line", () => + { + return grid.ChildrenOfType().Any(p => Precision.AlmostEquals(p.ScreenSpaceDrawQuad.TopRight.X, grid.ToScreenSpace(cursor.LastSnappedPosition).X)); + }); + } + [Test] public void TestLimitedDistance() { @@ -166,7 +201,7 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor RelativeSizeAxes = Axes.Both, Colour = Color4.SlateGray }, - new SnappingCursorContainer { GetSnapPosition = v => grid.GetSnappedPosition(grid.ToLocalSpace(v)).position }, + cursor = new SnappingCursorContainer { GetSnapPosition = v => grid.GetSnappedPosition(grid.ToLocalSpace(v)).position }, grid = new OsuDistanceSnapGrid(new HitCircle { Position = grid_position }, new HitCircle { StartTime = 200 }), }; }); @@ -186,6 +221,8 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor { public Func GetSnapPosition; + public Vector2 LastSnappedPosition { get; private set; } + private readonly Drawable cursor; private InputManager inputManager; @@ -214,7 +251,7 @@ namespace osu.Game.Rulesets.Osu.Tests.Editor protected override void Update() { base.Update(); - cursor.Position = GetSnapPosition.Invoke(inputManager.CurrentState.Mouse.Position); + cursor.Position = LastSnappedPosition = GetSnapPosition.Invoke(inputManager.CurrentState.Mouse.Position); } } } From 2016d330553bc1dc4ebbe3135011e71454590b8e Mon Sep 17 00:00:00 2001 From: Dean Herbert Date: Wed, 26 Oct 2022 13:10:39 +0900 Subject: [PATCH 21/21] Offset start of distance snap grid drawing if reference object's start time doesn't align Closes #20938. --- .../Edit/Compose/Components/CircularDistanceSnapGrid.cs | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/osu.Game/Screens/Edit/Compose/Components/CircularDistanceSnapGrid.cs b/osu.Game/Screens/Edit/Compose/Components/CircularDistanceSnapGrid.cs index 98079116cd..6e54e98740 100644 --- a/osu.Game/Screens/Edit/Compose/Components/CircularDistanceSnapGrid.cs +++ b/osu.Game/Screens/Edit/Compose/Components/CircularDistanceSnapGrid.cs @@ -53,9 +53,16 @@ namespace osu.Game.Screens.Edit.Compose.Components float maxDistance = new Vector2(dx, dy).Length; int requiredCircles = Math.Min(MaxIntervals, (int)(maxDistance / DistanceBetweenTicks)); + // We need to offset the drawn lines to the next valid snap for the currently selected divisor. + // + // Picture the scenario where the user has just placed an object on a 1/2 snap, then changes to + // 1/3 snap and expects to be able to place the next object on a valid 1/3 snap, regardless of the + // fact that the 1/2 snap reference object is not valid for 1/3 snapping. + float offset = SnapProvider.FindSnappedDistance(ReferenceObject, 0); + for (int i = 0; i < requiredCircles; i++) { - float diameter = (i + 1) * DistanceBetweenTicks * 2; + float diameter = (offset + (i + 1) * DistanceBetweenTicks) * 2; AddInternal(new Ring(ReferenceObject, GetColourForIndexFromPlacement(i)) {