Merge pull request #28739 from peppy/fix-skin-fallbacks

Fix incorrect skin fallback order when beatmap skin is present

osu.Game/Skinning/BeatmapSkinProvidingContainer.cs

@@ -1,8 +1,6 @@
 // Copyright (c) ppy Pty Ltd <contact@ppy.sh>. Licensed under the MIT Licence.
 // See the LICENCE file in the repository root for full licence text.
 
-#nullable disable
-
 using System;
 using osu.Framework.Allocation;
 using osu.Framework.Bindables;
@@ -17,9 +15,9 @@ namespace osu.Game.Skinning
     /// </summary>
     public partial class BeatmapSkinProvidingContainer : SkinProvidingContainer
     {
-        private Bindable<bool> beatmapSkins;
+        private Bindable<bool> beatmapSkins = null!;
-        private Bindable<bool> beatmapColours;
+        private Bindable<bool> beatmapColours = null!;
-        private Bindable<bool> beatmapHitsounds;
+        private Bindable<bool> beatmapHitsounds = null!;
 
         protected override bool AllowConfigurationLookup
         {
@@ -68,11 +66,15 @@ namespace osu.Game.Skinning
         }
 
         private readonly ISkin skin;
+        private readonly ISkin? classicFallback;
 
-        public BeatmapSkinProvidingContainer(ISkin skin)
+        private Bindable<Skin> currentSkin = null!;
+
+        public BeatmapSkinProvidingContainer(ISkin skin, ISkin? classicFallback = null)
             : base(skin)
         {
             this.skin = skin;
+            this.classicFallback = classicFallback;
         }
 
         protected override IReadOnlyDependencyContainer CreateChildDependencies(IReadOnlyDependencyContainer parent)
@@ -93,15 +95,27 @@ namespace osu.Game.Skinning
             beatmapColours.BindValueChanged(_ => TriggerSourceChanged());
             beatmapHitsounds.BindValueChanged(_ => TriggerSourceChanged());
 
-            // If the beatmap skin looks to have skinnable resources, add the default classic skin as a fallback opportunity.
+            currentSkin = skins.CurrentSkin.GetBoundCopy();
-            if (skin is LegacySkinTransformer legacySkin && legacySkin.IsProvidingLegacyResources)
+            currentSkin.BindValueChanged(_ =>
             {
-                SetSources(new[]
+                bool userSkinIsLegacy = skins.CurrentSkin.Value is LegacySkin;
-                {
+                bool beatmapProvidingResources = skin is LegacySkinTransformer legacySkin && legacySkin.IsProvidingLegacyResources;
-                    skin,
+
-                    skins.DefaultClassicSkin
+                // Some beatmaps provide a limited selection of skin elements to add some visual flair.
-                });
+                // In stable, these elements will take lookup priority over the selected skin (whether that be a user skin or default).
-            }
+                //
+                // To replicate this we need to pay special attention to the fallback order.
+                // If a user has a non-legacy skin (argon, triangles) selected, the game won't normally fall back to a legacy skin.
+                // In turn this can create an unexpected visual experience.
+                //
+                // So here, check what skin the user has selected. If it's already a legacy skin then we don't need to do anything special.
+                // If it isn't, we insert the classic default. Note that this is only done if the beatmap seems to be providing skin elements,
+                // as we only want to override the user's (non-legacy) skin choice when required for beatmap skin visuals.
+                if (!userSkinIsLegacy && beatmapProvidingResources && classicFallback != null)
+                    SetSources(new[] { skin, classicFallback });
+                else
+                    SetSources(new[] { skin });
+            }, true);
         }
     }
 }

osu.Game/Skinning/RulesetSkinProvidingContainer.cs

@@ -28,25 +28,33 @@ namespace osu.Game.Skinning
         protected readonly Ruleset Ruleset;
         protected readonly IBeatmap Beatmap;
 
+        [CanBeNull]
+        private readonly ISkin beatmapSkin;
+
         /// <remarks>
         /// This container already re-exposes all parent <see cref="ISkinSource"/> sources in a ruleset-usable form.
         /// Therefore disallow falling back to any parent <see cref="ISkinSource"/> any further.
         /// </remarks>
         protected override bool AllowFallingBackToParent => false;
 
-        protected override Container<Drawable> Content { get; }
+        protected override Container<Drawable> Content { get; } = new Container
+        {
+            RelativeSizeAxes = Axes.Both,
+        };
 
         public RulesetSkinProvidingContainer(Ruleset ruleset, IBeatmap beatmap, [CanBeNull] ISkin beatmapSkin)
         {
             Ruleset = ruleset;
             Beatmap = beatmap;
+            this.beatmapSkin = beatmapSkin;
+        }
 
-            InternalChild = new BeatmapSkinProvidingContainer(GetRulesetTransformedSkin(beatmapSkin))
+        [BackgroundDependencyLoader]
+        private void load(SkinManager skinManager)
+        {
+            InternalChild = new BeatmapSkinProvidingContainer(GetRulesetTransformedSkin(beatmapSkin), GetRulesetTransformedSkin(skinManager.DefaultClassicSkin))
             {
-                Child = Content = new Container
+                Child = Content,
-                {
-                    RelativeSizeAxes = Axes.Both,
-                }
             };
         }
 

osu.Game/Skinning/SkinProvidingContainer.cs

@@ -7,6 +7,7 @@ using System.Linq;
 using osu.Framework.Allocation;
 using osu.Framework.Audio.Sample;
 using osu.Framework.Bindables;
+using osu.Framework.Extensions.ObjectExtensions;
 using osu.Framework.Extensions.TypeExtensions;
 using osu.Framework.Graphics;
 using osu.Framework.Graphics.Containers;
@@ -201,7 +202,10 @@ namespace osu.Game.Skinning
                         source.SourceChanged -= TriggerSourceChanged;
                 }
 
-                skinSources = sources.Select(skin => (skin, new DisableableSkinSource(skin, this))).ToArray();
+                skinSources = sources
+                              // Shouldn't be required after NRT is applied to all calling sources.
+                              .Where(skin => skin.IsNotNull())
+                              .Select(skin => (skin, new DisableableSkinSource(skin, this))).ToArray();
 
                 foreach (var skin in skinSources)
                 {