Merge pull request #33300 from peppy/song-select-v2-fix-group-panel-recreation

SongSelectV2: Fix group panels being recreated every filter

osu.Game.Tests/Visual/SongSelectV2/TestSceneBeatmapCarouselUpdateHandling.cs

@@ -8,6 +8,7 @@ using NUnit.Framework;
 using osu.Framework.Testing;
 using osu.Game.Beatmaps;
 using osu.Game.Extensions;
+using osu.Game.Graphics.Sprites;
 using osu.Game.Screens.Select.Filter;
 using osu.Game.Screens.SelectV2;
 using osu.Game.Tests.Resources;
@@ -63,6 +64,8 @@ namespace osu.Game.Tests.Visual.SongSelectV2
         [Test]
         public void TestBeatmapSetMetadataUpdated()
         {
+            PanelBeatmapSet panel = null!;
+
             var metadata = new BeatmapMetadata
             {
                 Artist = "updated test",
@@ -77,10 +80,15 @@ namespace osu.Game.Tests.Visual.SongSelectV2
                 originalDrawables.AddRange(Carousel.ChildrenOfType<Panel>().ToList());
             });
 
+            AddStep("find panel", () => panel = Carousel.ChildrenOfType<PanelBeatmapSet>().Single(p => p.ChildrenOfType<OsuSpriteText>().Any(t => t.Text.ToString() == "beatmap")));
+
             updateBeatmap(b => b.Metadata = metadata);
 
             WaitForFiltering();
-            AddAssert("drawables changed", () => Carousel.ChildrenOfType<Panel>(), () => Is.Not.EqualTo(originalDrawables));
+
+            AddAssert("drawables unchanged", () => Carousel.ChildrenOfType<Panel>(), () => Is.EqualTo(originalDrawables));
+
+            AddAssert("title updated", () => panel.ChildrenOfType<OsuSpriteText>().Any(t => t.Text.ToString() == metadata.Title));
         }
 
         [Test]

osu.Game/Graphics/Carousel/Carousel.cs

@@ -756,12 +756,7 @@ namespace osu.Game.Graphics.Carousel
                     continue;
                 }
 
-                // The case where we're intending to display this panel, but it's already displayed.
-                // Note that we **must compare the model here** as the CarouselItems may be fresh instances due to a filter operation.
-                //
-                // Reference equality is used here instead of CheckModelEquality intentionally. In order to switch to `CheckModelEquality`,
-                // we need a way to signal to the drawable panels that there is an update.
-                var existing = toDisplay.FirstOrDefault(i => ReferenceEquals(i.Model, carouselPanel.Item!.Model));
+                var existing = toDisplay.FirstOrDefault(i => CheckModelEquality(i.Model, carouselPanel.Item!.Model));
 
                 if (existing != null)
                 {

osu.Game/Screens/SelectV2/BeatmapCarousel.cs

@@ -473,6 +473,12 @@ namespace osu.Game.Screens.SelectV2
             if (x is BeatmapInfo beatmapX && y is BeatmapInfo beatmapY)
                 return beatmapX.Equals(beatmapY);
 
+            if (x is GroupDefinition groupX && y is GroupDefinition groupY)
+                return groupX.Equals(groupY);
+
+            if (x is StarDifficultyGroupDefinition starX && y is StarDifficultyGroupDefinition starY)
+                return starX.Equals(starY);
+
             return base.CheckModelEquality(x, y);
         }
 

osu.Game/Screens/SelectV2/Panel.cs

@@ -216,7 +216,18 @@ namespace osu.Game.Screens.SelectV2
         protected override void PrepareForUse()
         {
             base.PrepareForUse();
-            this.FadeInFromZero(DURATION, Easing.OutQuint);
+
+            this.FadeIn(DURATION, Easing.OutQuint);
+        }
+
+        protected override void FreeAfterUse()
+        {
+            base.FreeAfterUse();
+
+            Hide();
+
+            // Important to set this to null to handle reuse scenarios correctly, see `Item` implementation.
+            item = null;
         }
 
         protected override bool OnClick(ClickEvent e)
@@ -283,7 +294,30 @@ namespace osu.Game.Screens.SelectV2
 
         #region ICarouselPanel
 
-        public CarouselItem? Item { get; set; }
+        private CarouselItem? item;
+
+        public CarouselItem? Item
+        {
+            get => item;
+            set
+            {
+                if (ReferenceEquals(item, value))
+                    return;
+
+                // If a new item is set and we already have an item, this is a case of reuse.
+                // To keep things simple, assume that we need to do a full refresh.
+                //
+                // In the future, this could be more contextual and check whether the associated model has actually changed.
+                if (item != null && value != null)
+                {
+                    item = value;
+                    PrepareForUse();
+                }
+                else
+                    item = value;
+            }
+        }
+
         public BindableBool Selected { get; } = new BindableBool();
         public BindableBool Expanded { get; } = new BindableBool();
         public BindableBool KeyboardSelected { get; } = new BindableBool();