Merge pull request ppy#23564 from peppy/fix-missing-sample-additions

Fix sample banks not transferring when adjusting via editor

Templates/Rulesets/ruleset-example/osu.Game.Rulesets.Pippidon/Objects/Drawables/DrawablePippidonHitObject.cs

@@ -8,7 +8,6 @@
 using osu.Framework.Graphics.Sprites;
 using osu.Framework.Graphics.Textures;
 using osu.Game.Audio;
-using osu.Game.Beatmaps.ControlPoints;
 using osu.Game.Rulesets.Objects.Drawables;
 using osu.Game.Rulesets.Scoring;
 using osuTK;
@@ -44,7 +43,7 @@ private void load(TextureStore textures)
 
         public override IEnumerable<HitSampleInfo> GetSamples() => new[]
         {
-            new HitSampleInfo(HitSampleInfo.HIT_NORMAL, SampleControlPoint.DEFAULT_BANK)
+            new HitSampleInfo(HitSampleInfo.HIT_NORMAL)
         };
 
         protected override void CheckForResult(bool userTriggered, double timeOffset)

Templates/Rulesets/ruleset-scrolling-example/osu.Game.Rulesets.Pippidon/Objects/Drawables/DrawablePippidonHitObject.cs

@@ -8,7 +8,6 @@
 using osu.Framework.Graphics.Sprites;
 using osu.Framework.Graphics.Textures;
 using osu.Game.Audio;
-using osu.Game.Beatmaps.ControlPoints;
 using osu.Game.Rulesets.Objects.Drawables;
 using osu.Game.Rulesets.Pippidon.UI;
 using osu.Game.Rulesets.Scoring;
@@ -44,7 +43,7 @@ private void load(PippidonPlayfield playfield, TextureStore textures)
 
         public override IEnumerable<HitSampleInfo> GetSamples() => new[]
         {
-            new HitSampleInfo(HitSampleInfo.HIT_NORMAL, SampleControlPoint.DEFAULT_BANK)
+            new HitSampleInfo(HitSampleInfo.HIT_NORMAL)
         };
 
         protected override void CheckForResult(bool userTriggered, double timeOffset)

osu.Game.Rulesets.Catch.Tests/TestSceneAutoJuiceStream.cs

@@ -45,7 +45,7 @@ protected override IBeatmap CreateBeatmap(RulesetInfo ruleset)
                     NewCombo = i % 8 == 0,
                     Samples = new List<HitSampleInfo>(new[]
                     {
-                        new HitSampleInfo(HitSampleInfo.HIT_NORMAL, "normal", volume: 100)
+                        new HitSampleInfo(HitSampleInfo.HIT_NORMAL)
                     })
                 });
             }

osu.Game.Rulesets.Catch/Objects/Banana.cs

@@ -58,7 +58,7 @@ public BananaHitSampleInfo(int volume = 100)
             {
             }
 
-            public sealed override HitSampleInfo With(Optional<string> newName = default, Optional<string?> newBank = default, Optional<string?> newSuffix = default, Optional<int> newVolume = default)
+            public sealed override HitSampleInfo With(Optional<string> newName = default, Optional<string> newBank = default, Optional<string?> newSuffix = default, Optional<int> newVolume = default)
                 => new BananaHitSampleInfo(newVolume.GetOr(Volume));
 
             public bool Equals(BananaHitSampleInfo? other)

osu.Game.Rulesets.Catch/Objects/BananaShower.cs

@@ -41,7 +41,7 @@ private void createBananas(CancellationToken cancellationToken)
                 {
                     StartTime = time,
                     BananaIndex = i,
-                    Samples = new List<HitSampleInfo> { new Banana.BananaHitSampleInfo(GetSampleInfo().Volume) }
+                    Samples = new List<HitSampleInfo> { new Banana.BananaHitSampleInfo(CreateHitSampleInfo().Volume) }
                 });
 
                 time += spacing;

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

@@ -74,7 +74,7 @@ private Drawable testSingle(float circleSize, bool auto = false, double length =
                 EndTime = Time.Current + delay + length,
                 Samples = new List<HitSampleInfo>
                 {
-                    new HitSampleInfo("hitnormal")
+                    new HitSampleInfo(HitSampleInfo.HIT_NORMAL)
                 }
             };
 

osu.Game.Rulesets.Osu/Objects/Spinner.cs

@@ -72,7 +72,7 @@ protected override void CreateNestedHitObjects(CancellationToken cancellationTok
 
                 AddNested(i < SpinsRequired
                     ? new SpinnerTick { StartTime = startTime, SpinnerDuration = Duration }
-                    : new SpinnerBonusTick { StartTime = startTime, SpinnerDuration = Duration, Samples = new[] { GetSampleInfo("spinnerbonus") } });
+                    : new SpinnerBonusTick { StartTime = startTime, SpinnerDuration = Duration, Samples = new[] { CreateHitSampleInfo("spinnerbonus") } });
             }
         }
 

osu.Game.Rulesets.Taiko/Objects/Hit.cs

@@ -63,7 +63,7 @@ private void updateSamplesFromType()
             if (isRimType != rimSamples.Any())
             {
                 if (isRimType)
-                    Samples.Add(new HitSampleInfo(HitSampleInfo.HIT_CLAP));
+                    Samples.Add(CreateHitSampleInfo(HitSampleInfo.HIT_CLAP));
                 else
                 {
                     foreach (var sample in rimSamples)

osu.Game.Rulesets.Taiko/Objects/TaikoStrongableHitObject.cs

@@ -56,7 +56,7 @@ private void updateSamplesFromType()
             if (IsStrongBindable.Value != strongSamples.Any())
             {
                 if (IsStrongBindable.Value)
-                    Samples.Add(GetSampleInfo(HitSampleInfo.HIT_FINISH));
+                    Samples.Add(CreateHitSampleInfo(HitSampleInfo.HIT_FINISH));
                 else
                 {
                     foreach (var sample in strongSamples)

osu.Game.Tests/Visual/Editing/TestSceneHitObjectSamplePointAdjustments.cs → osu.Game.Tests/Visual/Editing/TestSceneHitObjectSampleAdjustments.cs

@@ -24,7 +24,7 @@
 
 namespace osu.Game.Tests.Visual.Editing
 {
-    public partial class TestSceneHitObjectSamplePointAdjustments : EditorTestScene
+    public partial class TestSceneHitObjectSampleAdjustments : EditorTestScene
     {
         protected override Ruleset CreateEditorRuleset() => new OsuRuleset();
 
@@ -42,7 +42,7 @@ public override void SetUpSteps()
                     Position = (OsuPlayfield.BASE_SIZE - new Vector2(100, 0)) / 2,
                     Samples = new List<HitSampleInfo>
                     {
-                        new HitSampleInfo(HitSampleInfo.HIT_NORMAL, "normal", volume: 80)
+                        new HitSampleInfo(HitSampleInfo.HIT_NORMAL, volume: 80)
                     }
                 });
 
@@ -58,6 +58,26 @@ public override void SetUpSteps()
             });
         }
 
+        [Test]
+        public void TestAddSampleAddition()
+        {
+            AddStep("select both objects", () => EditorBeatmap.SelectedHitObjects.AddRange(EditorBeatmap.HitObjects));
+
+            AddStep("add clap addition", () => InputManager.Key(Key.R));
+
+            hitObjectHasSampleBank(0, "normal");
+            hitObjectHasSamples(0, HitSampleInfo.HIT_NORMAL, HitSampleInfo.HIT_CLAP);
+            hitObjectHasSampleBank(1, "soft");
+            hitObjectHasSamples(1, HitSampleInfo.HIT_NORMAL, HitSampleInfo.HIT_CLAP);
+
+            AddStep("remove clap addition", () => InputManager.Key(Key.R));
+
+            hitObjectHasSampleBank(0, "normal");
+            hitObjectHasSamples(0, HitSampleInfo.HIT_NORMAL);
+            hitObjectHasSampleBank(1, "soft");
+            hitObjectHasSamples(1, HitSampleInfo.HIT_NORMAL);
+        }
+
         [Test]
         public void TestPopoverHasFocus()
         {
@@ -271,6 +291,12 @@ private void setBankViaPopover(string bank) => AddStep($"set bank {bank} via pop
             InputManager.Key(Key.Enter);
         });
 
+        private void hitObjectHasSamples(int objectIndex, params string[] samples) => AddAssert($"{objectIndex.ToOrdinalWords()} has samples {string.Join(',', samples)}", () =>
+        {
+            var h = EditorBeatmap.HitObjects.ElementAt(objectIndex);
+            return h.Samples.Select(s => s.Name).SequenceEqual(samples);
+        });
+
         private void hitObjectHasSampleBank(int objectIndex, string bank) => AddAssert($"{objectIndex.ToOrdinalWords()} has bank {bank}", () =>
         {
             var h = EditorBeatmap.HitObjects.ElementAt(objectIndex);

osu.Game/Audio/HitSampleInfo.cs

@@ -4,6 +4,7 @@
 using System;
 using System.Collections.Generic;
 using Newtonsoft.Json;
+using osu.Game.Beatmaps.ControlPoints;
 using osu.Game.Utils;
 
 namespace osu.Game.Audio
@@ -32,7 +33,7 @@ public class HitSampleInfo : ISampleInfo, IEquatable<HitSampleInfo>
         /// <summary>
         /// The bank to load the sample from.
         /// </summary>
-        public readonly string? Bank;
+        public readonly string Bank;
 
         /// <summary>
         /// An optional suffix to provide priority lookup. Falls back to non-suffixed <see cref="Name"/>.
@@ -44,7 +45,7 @@ public class HitSampleInfo : ISampleInfo, IEquatable<HitSampleInfo>
         /// </summary>
         public int Volume { get; }
 
-        public HitSampleInfo(string name, string? bank = null, string? suffix = null, int volume = 0)
+        public HitSampleInfo(string name, string bank = SampleControlPoint.DEFAULT_BANK, string? suffix = null, int volume = 100)
         {
             Name = name;
             Bank = bank;
@@ -75,7 +76,7 @@ public virtual IEnumerable<string> LookupNames
         /// <param name="newSuffix">An optional new lookup suffix.</param>
         /// <param name="newVolume">An optional new volume.</param>
         /// <returns>The new <see cref="HitSampleInfo"/>.</returns>
-        public virtual HitSampleInfo With(Optional<string> newName = default, Optional<string?> newBank = default, Optional<string?> newSuffix = default, Optional<int> newVolume = default)
+        public virtual HitSampleInfo With(Optional<string> newName = default, Optional<string> newBank = default, Optional<string?> newSuffix = default, Optional<int> newVolume = default)
             => new HitSampleInfo(newName.GetOr(Name), newBank.GetOr(Bank), newSuffix.GetOr(Suffix), newVolume.GetOr(Volume));
 
         public bool Equals(HitSampleInfo? other)

osu.Game/Beatmaps/ControlPoints/SampleControlPoint.cs

@@ -69,7 +69,7 @@ public int SampleVolume
         /// <param name="hitSampleInfo">The <see cref="HitSampleInfo"/>. This will not be modified.</param>
         /// <returns>The modified <see cref="HitSampleInfo"/>. This does not share a reference with <paramref name="hitSampleInfo"/>.</returns>
         public virtual HitSampleInfo ApplyTo(HitSampleInfo hitSampleInfo)
-            => hitSampleInfo.With(newBank: hitSampleInfo.Bank ?? SampleBank, newVolume: hitSampleInfo.Volume > 0 ? hitSampleInfo.Volume : SampleVolume);
+            => hitSampleInfo.With(newBank: hitSampleInfo.Bank, newVolume: hitSampleInfo.Volume > 0 ? hitSampleInfo.Volume : SampleVolume);
 
         public override bool IsRedundant(ControlPoint? existing)
             => existing is SampleControlPoint existingSample

osu.Game/Beatmaps/Formats/LegacyDecoder.cs

@@ -226,12 +226,16 @@ internal class LegacySampleControlPoint : SampleControlPoint, IEquatable<LegacyS
 
             public override HitSampleInfo ApplyTo(HitSampleInfo hitSampleInfo)
             {
-                var baseInfo = base.ApplyTo(hitSampleInfo);
-
-                if (baseInfo is ConvertHitObjectParser.LegacyHitSampleInfo legacy && legacy.CustomSampleBank == 0)
-                    return legacy.With(newCustomSampleBank: CustomSampleBank);
+                if (hitSampleInfo is ConvertHitObjectParser.LegacyHitSampleInfo legacy)
+                {
+                    return legacy.With(
+                        newCustomSampleBank: legacy.CustomSampleBank > 0 ? legacy.CustomSampleBank : CustomSampleBank,
+                        newVolume: hitSampleInfo.Volume > 0 ? hitSampleInfo.Volume : SampleVolume,
+                        newBank: legacy.BankSpecified ? legacy.Bank : SampleBank
+                    );
+                }
 
-                return baseInfo;
+                return base.ApplyTo(hitSampleInfo);
             }
 
             public override bool IsRedundant(ControlPoint? existing)

osu.Game/Rulesets/Edit/PlacementBlueprint.cs

@@ -63,7 +63,7 @@ protected PlacementBlueprint(HitObject hitObject)
             HitObject = hitObject;
 
             // adding the default hit sample should be the case regardless of the ruleset.
-            HitObject.Samples.Add(new HitSampleInfo(HitSampleInfo.HIT_NORMAL, SampleControlPoint.DEFAULT_BANK, volume: 100));
+            HitObject.Samples.Add(new HitSampleInfo(HitSampleInfo.HIT_NORMAL));
 
             RelativeSizeAxes = Axes.Both;
 

osu.Game/Rulesets/Objects/HitObject.cs

@@ -206,14 +206,20 @@ public IList<HitSampleInfo> CreateSlidingSamples()
         }
 
         /// <summary>
-        /// Create a SampleInfo based on the sample settings of the hit normal sample in <see cref="Samples"/>.
+        /// Create a <see cref="HitSampleInfo"/> based on the sample settings of the first <see cref="HitSampleInfo.HIT_NORMAL"/> sample in <see cref="Samples"/>.
+        /// If no sample is available, sane default settings will be used instead.
         /// </summary>
+        /// <remarks>
+        /// In the case an existing sample exists, all settings apart from the sample name will be inherited. This includes volume, bank and suffix.
+        /// </remarks>
         /// <param name="sampleName">The name of the sample.</param>
         /// <returns>A populated <see cref="HitSampleInfo"/>.</returns>
-        protected HitSampleInfo GetSampleInfo(string sampleName = HitSampleInfo.HIT_NORMAL)
+        public HitSampleInfo CreateHitSampleInfo(string sampleName = HitSampleInfo.HIT_NORMAL)
         {
-            var hitnormalSample = Samples.FirstOrDefault(s => s.Name == HitSampleInfo.HIT_NORMAL);
-            return hitnormalSample == null ? new HitSampleInfo(sampleName) : hitnormalSample.With(newName: sampleName);
+            if (Samples.FirstOrDefault(s => s.Name == HitSampleInfo.HIT_NORMAL) is HitSampleInfo existingSample)
+                return existingSample.With(newName: sampleName);
+
+            return new HitSampleInfo(sampleName);
         }
     }
 

osu.Game/Rulesets/Objects/Legacy/ConvertHitObjectParser.cs

@@ -14,6 +14,7 @@
 using JetBrains.Annotations;
 using osu.Framework.Extensions.EnumExtensions;
 using osu.Framework.Utils;
+using osu.Game.Beatmaps.ControlPoints;
 using osu.Game.Beatmaps.Legacy;
 using osu.Game.Skinning;
 using osu.Game.Utils;
@@ -446,9 +447,9 @@ private List<HitSampleInfo> convertSoundType(LegacyHitSoundType type, SampleBank
             if (string.IsNullOrEmpty(bankInfo.Filename))
             {
                 soundTypes.Add(new LegacyHitSampleInfo(HitSampleInfo.HIT_NORMAL, bankInfo.BankForNormal, bankInfo.Volume, bankInfo.CustomSampleBank,
-                                // if the sound type doesn't have the Normal flag set, attach it anyway as a layered sample.
-                                // None also counts as a normal non-layered sample: https://osu.ppy.sh/help/wiki/osu!_File_Formats/Osu_(file_format)#hitsounds
-                                type != LegacyHitSoundType.None && !type.HasFlagFast(LegacyHitSoundType.Normal)));
+                    // if the sound type doesn't have the Normal flag set, attach it anyway as a layered sample.
+                    // None also counts as a normal non-layered sample: https://osu.ppy.sh/help/wiki/osu!_File_Formats/Osu_(file_format)#hitsounds
+                    type != LegacyHitSoundType.None && !type.HasFlagFast(LegacyHitSoundType.Normal)));
             }
             else
             {
@@ -479,12 +480,14 @@ private class SampleBankInfo
             /// The bank identifier to use for the base ("hitnormal") sample.
             /// Transferred to <see cref="HitSampleInfo.Bank"/> when appropriate.
             /// </summary>
+            [CanBeNull]
             public string BankForNormal;
 
             /// <summary>
             /// The bank identifier to use for additions ("hitwhistle", "hitfinish", "hitclap").
             /// Transferred to <see cref="HitSampleInfo.Bank"/> when appropriate.
             /// </summary>
+            [CanBeNull]
             public string BankForAdditions;
 
             /// <summary>
@@ -518,17 +521,24 @@ public class LegacyHitSampleInfo : HitSampleInfo, IEquatable<LegacyHitSampleInfo
             /// </remarks>
             public readonly bool IsLayered;
 
+            /// <summary>
+            /// Whether a bank was specified locally to the relevant hitobject.
+            /// If <c>false</c>, a bank will be retrieved from the closest control point.
+            /// </summary>
+            public bool BankSpecified;
+
             public LegacyHitSampleInfo(string name, string? bank = null, int volume = 0, int customSampleBank = 0, bool isLayered = false)
-                : base(name, bank, customSampleBank >= 2 ? customSampleBank.ToString() : null, volume)
+                : base(name, bank ?? SampleControlPoint.DEFAULT_BANK, customSampleBank >= 2 ? customSampleBank.ToString() : null, volume)
             {
                 CustomSampleBank = customSampleBank;
+                BankSpecified = !string.IsNullOrEmpty(bank);
                 IsLayered = isLayered;
             }
 
-            public sealed override HitSampleInfo With(Optional<string> newName = default, Optional<string?> newBank = default, Optional<string?> newSuffix = default, Optional<int> newVolume = default)
+            public sealed override HitSampleInfo With(Optional<string> newName = default, Optional<string> newBank = default, Optional<string?> newSuffix = default, Optional<int> newVolume = default)
                 => With(newName, newBank, newVolume);
 
-            public virtual LegacyHitSampleInfo With(Optional<string> newName = default, Optional<string?> newBank = default, Optional<int> newVolume = default,
+            public virtual LegacyHitSampleInfo With(Optional<string> newName = default, Optional<string> newBank = default, Optional<int> newVolume = default,
                                                     Optional<int> newCustomSampleBank = default,
                                                     Optional<bool> newIsLayered = default)
                 => new LegacyHitSampleInfo(newName.GetOr(Name), newBank.GetOr(Bank), newVolume.GetOr(Volume), newCustomSampleBank.GetOr(CustomSampleBank), newIsLayered.GetOr(IsLayered));
@@ -563,7 +573,7 @@ public FileHitSampleInfo(string filename, int volume)
                 Path.ChangeExtension(Filename, null)
             };
 
-            public sealed override LegacyHitSampleInfo With(Optional<string> newName = default, Optional<string?> newBank = default, Optional<int> newVolume = default,
+            public sealed override LegacyHitSampleInfo With(Optional<string> newName = default, Optional<string> newBank = default, Optional<int> newVolume = default,
                                                             Optional<int> newCustomSampleBank = default,
                                                             Optional<bool> newIsLayered = default)
                 => new FileHitSampleInfo(Filename, newVolume.GetOr(Volume));

osu.Game/Screens/Edit/Compose/Components/ComposeBlueprintContainer.cs

@@ -185,7 +185,7 @@ private void sampleChanged(string sampleName, TernaryState state)
 
                 case TernaryState.True:
                     if (existingSample == null)
-                        samples.Add(new HitSampleInfo(sampleName));
+                        samples.Add(CurrentPlacement.HitObject.CreateHitSampleInfo(sampleName));
                     break;
             }
         }

osu.Game/Screens/Edit/Compose/Components/EditorSelectionHandler.cs

@@ -122,7 +122,7 @@ public void AddHitSample(string sampleName)
                 if (h.Samples.Any(s => s.Name == sampleName))
                     return;
 
-                h.Samples.Add(new HitSampleInfo(sampleName));
+                h.Samples.Add(h.CreateHitSampleInfo(sampleName));
                 EditorBeatmap.Update(h);
             });
         }