Merge pull request #33908 from bdach/custom-sample-bank-buggery

Fix beatmap skin sample lookups falling back to non-custom sample banks if the custom bank sample was not found

Author: peppy

osu.Game.Rulesets.Osu.Tests/TestSceneOsuHitObjectSamples.cs

@@ -14,26 +14,27 @@ public partial class TestSceneOsuHitObjectSamples : HitObjectSampleTest
 
         protected override IResourceStore<byte[]> RulesetResources => new DllResourceStore(Assembly.GetAssembly(typeof(TestSceneOsuHitObjectSamples)));
 
-        [TestCase("normal-hitnormal")]
-        [TestCase("hitnormal")]
-        public void TestDefaultCustomSampleFromBeatmap(string expectedSample)
+        [TestCase("normal-hitnormal2", "normal-hitnormal")]
+        [TestCase("hitnormal", "hitnormal")]
+        public void TestDefaultCustomSampleFromBeatmap(string beatmapSkinSampleName, string userSkinSampleName)
         {
-            SetupSkins(expectedSample, expectedSample);
+            SetupSkins(beatmapSkinSampleName, userSkinSampleName);
 
             CreateTestWithBeatmap("osu-hitobject-beatmap-custom-sample-bank.osu");
 
-            AssertBeatmapLookup(expectedSample);
+            AssertBeatmapLookup(beatmapSkinSampleName);
         }
 
-        [TestCase("normal-hitnormal")]
-        [TestCase("hitnormal")]
-        public void TestDefaultCustomSampleFromUserSkinFallback(string expectedSample)
+        [TestCase("", "normal-hitnormal")]
+        [TestCase("normal-hitnormal", "normal-hitnormal")]
+        [TestCase("", "hitnormal")]
+        public void TestDefaultCustomSampleFromUserSkinFallback(string beatmapSkinSampleName, string userSkinSampleName)
         {
-            SetupSkins(string.Empty, expectedSample);
+            SetupSkins(beatmapSkinSampleName, userSkinSampleName);
 
             CreateTestWithBeatmap("osu-hitobject-beatmap-custom-sample-bank.osu");
 
-            AssertUserLookup(expectedSample);
+            AssertUserLookup(userSkinSampleName);
         }
 
         [TestCase("normal-hitnormal2")]

osu.Game.Rulesets.Taiko.Tests/TestSceneTaikoHitObjectSamples.cs

@@ -14,26 +14,27 @@ public partial class TestSceneTaikoHitObjectSamples : HitObjectSampleTest
 
         protected override IResourceStore<byte[]> RulesetResources => new DllResourceStore(Assembly.GetAssembly(typeof(TestSceneTaikoHitObjectSamples)));
 
-        [TestCase("taiko-normal-hitnormal")]
-        [TestCase("hitnormal")]
-        public void TestDefaultCustomSampleFromBeatmap(string expectedSample)
+        [TestCase("taiko-normal-hitnormal2", "taiko-normal-hitnormal")]
+        [TestCase("hitnormal", "hitnormal")]
+        public void TestDefaultCustomSampleFromBeatmap(string beatmapSkinSampleName, string userSkinSampleName)
         {
-            SetupSkins(expectedSample, expectedSample);
+            SetupSkins(beatmapSkinSampleName, userSkinSampleName);
 
             CreateTestWithBeatmap("taiko-hitobject-beatmap-custom-sample-bank.osu");
 
-            AssertBeatmapLookup(expectedSample);
+            AssertBeatmapLookup(beatmapSkinSampleName);
         }
 
-        [TestCase("taiko-normal-hitnormal")]
-        [TestCase("hitnormal")]
-        public void TestDefaultCustomSampleFromUserSkinFallback(string expectedSample)
+        [TestCase("", "taiko-normal-hitnormal")]
+        [TestCase("taiko-normal-hitnormal", "taiko-normal-hitnormal")]
+        [TestCase("", "hitnormal")]
+        public void TestDefaultCustomSampleFromUserSkinFallback(string beatmapSkinSampleName, string userSkinSampleName)
         {
-            SetupSkins(string.Empty, expectedSample);
+            SetupSkins(beatmapSkinSampleName, userSkinSampleName);
 
             CreateTestWithBeatmap("taiko-hitobject-beatmap-custom-sample-bank.osu");
 
-            AssertUserLookup(expectedSample);
+            AssertUserLookup(userSkinSampleName);
         }
 
         [TestCase("taiko-normal-hitnormal2")]

osu.Game.Tests/Gameplay/TestSceneHitObjectSamples.cs

@@ -64,11 +64,9 @@ public void TestDefaultSampleFromUserSkinFallback()
         /// <summary>
         /// Tests that a hitobject which provides a custom sample set of 2 retrieves the following samples from the beatmap skin:
         /// normal-hitnormal2
-        /// normal-hitnormal
         /// hitnormal
         /// </summary>
         [TestCase("normal-hitnormal2")]
-        [TestCase("normal-hitnormal")]
         [TestCase("hitnormal")]
         public void TestDefaultCustomSampleFromBeatmap(string expectedSample)
         {
@@ -162,7 +160,6 @@ public void TestControlPointSampleFromBeatmap()
         /// Tests that a control point that provides a custom sample of 2 causes <see cref="TestDefaultCustomSampleFromBeatmap"/>.
         /// </summary>
         [TestCase("normal-hitnormal2")]
-        [TestCase("normal-hitnormal")]
         [TestCase("hitnormal")]
         public void TestControlPointCustomSampleFromBeatmap(string sampleName)
         {

osu.Game/Skinning/LegacySkin.cs

@@ -594,12 +594,17 @@ private IEnumerable<string> getLegacyLookupNames(HitSampleInfo hitSample)
         {
             var lookupNames = hitSample.LookupNames.SelectMany(getFallbackSampleNames);
 
-            if (!UseCustomSampleBanks && !string.IsNullOrEmpty(hitSample.Suffix))
+            if (!string.IsNullOrEmpty(hitSample.Suffix))
             {
-                // for compatibility with stable, exclude the lookup names with the custom sample bank suffix, if they are not valid for use in this skin.
+                // for compatibility with stable:
+                // - if the skin can use custom sample banks, it MUST use the custom sample bank suffix. it is not allowed to fall back to a non-custom sound.
+                // - if the skin cannot use custom sample banks, it MUST NOT use the custom sample bank suffix.
                 // using .EndsWith() is intentional as it ensures parity in all edge cases
-                // (see LegacyTaikoSampleInfo for an example of one - prioritising the taiko prefix should still apply, but the sample bank should not).
-                lookupNames = lookupNames.Where(name => !name.EndsWith(hitSample.Suffix, StringComparison.Ordinal));
+                // (see LegacyTaikoSampleInfo for an example of one - prioritising the taiko prefix should still apply).
+                if (UseCustomSampleBanks)
+                    lookupNames = lookupNames.Where(name => name.EndsWith(hitSample.Suffix, StringComparison.Ordinal));
+                else
+                    lookupNames = lookupNames.Where(name => !name.EndsWith(hitSample.Suffix, StringComparison.Ordinal));
             }
 
             foreach (string l in lookupNames)