Samples are assigned incorrectly to sliders

[peppy]

Originally "fixed" in #3809, but I'd argue this just shifted the problem to other beatmaps and fixed the couple of cases brought up in the issue thread.

Basically, in the linked discussion the slider is taking on a SampleControlPoint change before the end of the slider, while stable will always use point-in-time (aka, in the majority case, the control point at the StartTime).

To actually fix this properly, we likely want to apply the apply sample flow to nested hitobjects, allowing them to get correct samples for their StartTime. We'd also need to potentially allow sliders to lookup the correct sample point at their StartTime (for slide etc.) and EndTime (for final hit), but this edge case could also be avoided if the responsiblity of tail sample playback is shifted to the actual tail (see comment in #24966).

High effort to fix this one. Likely need to knock some serious sense into the sample lookup code.

Discussed in #24977

^{Originally posted by MaxKruse October 1, 2023}
As seen on this beatmap: https://osu.ppy.sh/beatmapsets/355573#osu/784791

soft-sliderslide99.wav ( 01:13:417 (1) - ) is not being played on lazer, but it being played on stable.

I havent checked if this is also the case with other samples, i just noticed this one while playing.

Replicate:

1. Change settings to use beatmap hitsounds
2. Pick any skin
3. Forward to the object in question
4. Sample isnt played

[peppy]

This can also cause slider ticks to get an incorrect volume (see #25245 for one example).

Improving the situation may be as easy as applying samples to nested objects in the decoder flow?

[bdach]

Improving the situation may be as easy as applying samples to nested objects in the decoder flow?

That will not fly for editor as nested objects are considered ephemeral and can be deleted at any time. For the volume to stay assigned you'd want to store those momentary volume changes somewhere on the slider (like NodeSamples but even more granular).

[peppy]

For sure, that's why I said "improving" and not "fixing". I think it could potentially fix legacy beatmap loading at least. Might be worth it as a temporary solution if it's a few lines change?

[bdach]

I guess. So long as it's signposted properly via inline comments or otherwise.

[peppy]

I've made this work, but it requires some pretty... ugly logic. Maybe if there's a cleaner way to get the sample points required that I'm missing it can still work, but not sure.

diff --git a/osu.Game.Rulesets.Osu/Objects/Slider.cs b/osu.Game.Rulesets.Osu/Objects/Slider.cs
index c62659d67a..c41a2f981e 100644
--- a/osu.Game.Rulesets.Osu/Objects/Slider.cs
+++ b/osu.Game.Rulesets.Osu/Objects/Slider.cs
@@ -15,6 +15,8 @@
 using osu.Game.Audio;
 using osu.Game.Beatmaps;
 using osu.Game.Beatmaps.ControlPoints;
+using osu.Game.Beatmaps.Formats;
+using osu.Game.Beatmaps.Legacy;
 using osu.Game.Rulesets.Judgements;
 using osu.Game.Rulesets.Objects.Legacy;
 using osu.Game.Rulesets.Osu.Judgements;
@@ -133,6 +135,8 @@ public int RepeatCount
         /// </summary>
         public bool OnlyJudgeNestedObjects = true;
 
+        private ControlPointInfo controlPointInfo;
+
         public BindableNumber<double> SliderVelocityMultiplierBindable { get; } = new BindableDouble(1)
         {
             MinValue = 0.1,
@@ -163,6 +167,8 @@ protected override void ApplyDefaultsToSelf(ControlPointInfo controlPointInfo, I
         {
             base.ApplyDefaultsToSelf(controlPointInfo, difficulty);
 
+            this.controlPointInfo = controlPointInfo;
+
             TimingControlPoint timingPoint = controlPointInfo.TimingPointAt(StartTime);
 
             Velocity = BASE_SCORING_DISTANCE * difficulty.SliderMultiplier / LegacyRulesetExtensions.GetPrecisionAdjustedBeatLength(this, timingPoint, OsuRuleset.SHORT_NAME);
@@ -245,13 +251,20 @@ protected void UpdateNestedSamples()
         {
             var firstSample = Samples.FirstOrDefault(s => s.Name == HitSampleInfo.HIT_NORMAL)
                               ?? Samples.FirstOrDefault(); // TODO: remove this when guaranteed sort is present for samples (https://github.com/ppy/osu/issues/1933)
-            var sampleList = new List<HitSampleInfo>();
-
-            if (firstSample != null)
-                sampleList.Add(firstSample.With("slidertick"));
 
             foreach (var tick in NestedHitObjects.OfType<SliderTick>())
-                tick.Samples = sampleList;
+            {
+                if (firstSample != null)
+                {
+                    if (controlPointInfo is LegacyControlPointInfo legacyControlPointInfo)
+                    {
+                        var samplePoint = legacyControlPointInfo.SamplePointAt(tick.GetEndTime() + LegacyBeatmapDecoder.CONTROL_POINT_LENIENCY);
+                        tick.Samples = new List<HitSampleInfo> { firstSample.With("slidertick", newVolume: samplePoint.SampleVolume) };
+                    }
+                    else
+                        tick.Samples = new List<HitSampleInfo> { firstSample.With("slidertick") };
+                }
+            }
 
             foreach (var repeat in NestedHitObjects.OfType<SliderRepeat>())
                 repeat.Samples = this.GetNodeSamples(repeat.RepeatIndex + 1);
diff --git a/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs b/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs
index 8c5e4971d5..d38dd60cd1 100644
--- a/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs
+++ b/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs
@@ -33,7 +33,7 @@ public class LegacyBeatmapDecoder : LegacyDecoder<Beatmap>
         /// <remarks>
         /// Compare: https://github.com/peppy/osu-stable-reference/blob/master/osu!/GameplayElements/HitObjects/HitObject.cs#L319
         /// </remarks>
-        private const double control_point_leniency = 5;
+        public const double CONTROL_POINT_LENIENCY = 5;
 
         internal static RulesetStore? RulesetStore;
 
@@ -115,7 +115,7 @@ private void applyDefaults(HitObject hitObject)
 
         private void applySamples(HitObject hitObject)
         {
-            SampleControlPoint sampleControlPoint = (beatmap.ControlPointInfo as LegacyControlPointInfo)?.SamplePointAt(hitObject.GetEndTime() + control_point_leniency) ?? SampleControlPoint.DEFAULT;
+            SampleControlPoint sampleControlPoint = (beatmap.ControlPointInfo as LegacyControlPointInfo)?.SamplePointAt(hitObject.GetEndTime() + CONTROL_POINT_LENIENCY) ?? SampleControlPoint.DEFAULT;
 
             hitObject.Samples = hitObject.Samples.Select(o => sampleControlPoint.ApplyTo(o)).ToList();
 
@@ -123,7 +123,7 @@ private void applySamples(HitObject hitObject)
             {
                 for (int i = 0; i < hasRepeats.NodeSamples.Count; i++)
                 {
-                    double time = hitObject.StartTime + i * hasRepeats.Duration / hasRepeats.SpanCount() + control_point_leniency;
+                    double time = hitObject.StartTime + i * hasRepeats.Duration / hasRepeats.SpanCount() + CONTROL_POINT_LENIENCY;
                     var nodeSamplePoint = (beatmap.ControlPointInfo as LegacyControlPointInfo)?.SamplePointAt(time) ?? SampleControlPoint.DEFAULT;
 
                     hasRepeats.NodeSamples[i] = hasRepeats.NodeSamples[i].Select(o => nodeSamplePoint.ApplyTo(o)).ToList();

[bdach]

This is fixed now and I'd wager that the change that fixed this is #28619.