From fff27e619d41802702722f7426b8000c5f1ebd6a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Bart=C5=82omiej=20Dach?= Date: Wed, 26 Jun 2024 14:27:14 +0200 Subject: [PATCH] Fix slider tail volume not saving Closes https://github.com/ppy/osu/issues/28587. As outlined in the issue thread, the tail volume wasn't saving because it wasn't actually attached to a hitobject properly, and as such the `LegacyBeatmapEncoder` logic, which is based on hitobjects, did not pick them up on save. To fix that, switch to using `NodeSamples` for objects that are `IHasRepeats`. That has one added complication in that having it work properly requires changes to the decode side too. That is because the intent is to allow the user to change the sample settings for each node (which are specified via `NodeSamples`), as well as "the rest of the object", which generally means ticks or auxiliary samples like `sliderslide` (which are specified by `Samples`). However, up until now, `Samples` always queried the control point which was _active at the end time of the slider_. This obviously can't work anymore when converting `NodeSamples` to legacy control points, because the last node's sample is _also_ at the end time of the slider. To bypass that, add extra sample points after each node (just out of reach of the 5ms leniency), which are supposed to control volume of ticks and/or slides. Upon testing, this *sort of* has the intended effect in stable, with the exception of `sliderslide`, which seems to either respect or _not_ respect the relevant volume spec dependent on... not sure what, and not sure I want to be debugging that. It might be frame alignment, or it might be the phase of the moon. --- .../Formats/LegacyBeatmapDecoderTest.cs | 13 ++++++-- .../Beatmaps/Formats/LegacyBeatmapDecoder.cs | 16 ++++++---- .../Beatmaps/Formats/LegacyBeatmapEncoder.cs | 32 +++++++++++++++---- 3 files changed, 47 insertions(+), 14 deletions(-) diff --git a/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs b/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs index a4cd888823..19378821b3 100644 --- a/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs +++ b/osu.Game.Tests/Beatmaps/Formats/LegacyBeatmapDecoderTest.cs @@ -528,8 +528,17 @@ namespace osu.Game.Tests.Beatmaps.Formats Assert.AreEqual("Gameplay/normal-hitnormal2", getTestableSampleInfo(hitObjects[2]).LookupNames.First()); Assert.AreEqual("Gameplay/normal-hitnormal", getTestableSampleInfo(hitObjects[3]).LookupNames.First()); - // The control point at the end time of the slider should be applied - Assert.AreEqual("Gameplay/soft-hitnormal8", getTestableSampleInfo(hitObjects[4]).LookupNames.First()); + // The fourth object is a slider. + // `Samples` of a slider are presumed to control the volume of sounds that last the entire duration of the slider + // (such as ticks, slider slide sounds, etc.) + // Thus, the point of query of control points used for `Samples` is just beyond the start time of the slider. + Assert.AreEqual("Gameplay/soft-hitnormal11", getTestableSampleInfo(hitObjects[4]).LookupNames.First()); + + // That said, the `NodeSamples` of the slider are responsible for the sounds of the slider's head / tail / repeats / large ticks etc. + // Therefore, they should be read at the time instant correspondent to the given node. + // This means that the tail should use bank 8 rather than 11. + Assert.AreEqual("Gameplay/soft-hitnormal11", ((ConvertSlider)hitObjects[4]).NodeSamples[0][0].LookupNames.First()); + Assert.AreEqual("Gameplay/soft-hitnormal8", ((ConvertSlider)hitObjects[4]).NodeSamples[1][0].LookupNames.First()); } static HitSampleInfo getTestableSampleInfo(HitObject hitObject) => hitObject.Samples[0]; diff --git a/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs b/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs index c2f4097889..5fa85f189c 100644 --- a/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs +++ b/osu.Game/Beatmaps/Formats/LegacyBeatmapDecoder.cs @@ -33,7 +33,7 @@ namespace osu.Game.Beatmaps.Formats /// /// Compare: https://github.com/peppy/osu-stable-reference/blob/master/osu!/GameplayElements/HitObjects/HitObject.cs#L319 /// - private const double control_point_leniency = 5; + public const double CONTROL_POINT_LENIENCY = 5; internal static RulesetStore? RulesetStore; @@ -160,20 +160,24 @@ namespace osu.Game.Beatmaps.Formats private void applySamples(HitObject hitObject) { - SampleControlPoint sampleControlPoint = (beatmap.ControlPointInfo as LegacyControlPointInfo)?.SamplePointAt(hitObject.GetEndTime() + control_point_leniency) ?? SampleControlPoint.DEFAULT; - - hitObject.Samples = hitObject.Samples.Select(o => sampleControlPoint.ApplyTo(o)).ToList(); - if (hitObject is IHasRepeats hasRepeats) { + SampleControlPoint sampleControlPoint = (beatmap.ControlPointInfo as LegacyControlPointInfo)?.SamplePointAt(hitObject.StartTime + CONTROL_POINT_LENIENCY + 1) ?? SampleControlPoint.DEFAULT; + hitObject.Samples = hitObject.Samples.Select(o => sampleControlPoint.ApplyTo(o)).ToList(); + 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(); } } + else + { + SampleControlPoint sampleControlPoint = (beatmap.ControlPointInfo as LegacyControlPointInfo)?.SamplePointAt(hitObject.GetEndTime() + CONTROL_POINT_LENIENCY) ?? SampleControlPoint.DEFAULT; + hitObject.Samples = hitObject.Samples.Select(o => sampleControlPoint.ApplyTo(o)).ToList(); + } } /// diff --git a/osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs b/osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs index 186b565c39..09e3150359 100644 --- a/osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs +++ b/osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs @@ -282,19 +282,39 @@ namespace osu.Game.Beatmaps.Formats { foreach (var hitObject in hitObjects) { - if (hitObject.Samples.Count > 0) + if (hitObject is IHasRepeats hasNodeSamples) { - int volume = hitObject.Samples.Max(o => o.Volume); - int customIndex = hitObject.Samples.Any(o => o is ConvertHitObjectParser.LegacyHitSampleInfo) - ? hitObject.Samples.OfType().Max(o => o.CustomSampleBank) - : -1; + double spanDuration = hasNodeSamples.Duration / hasNodeSamples.SpanCount(); - yield return new LegacyBeatmapDecoder.LegacySampleControlPoint { Time = hitObject.GetEndTime(), SampleVolume = volume, CustomSampleBank = customIndex }; + for (int i = 0; i < hasNodeSamples.NodeSamples.Count; ++i) + { + double nodeTime = hitObject.StartTime + i * spanDuration; + + if (hasNodeSamples.NodeSamples[i].Any()) + yield return createSampleControlPointFor(nodeTime, hasNodeSamples.NodeSamples[i]); + + if (spanDuration > LegacyBeatmapDecoder.CONTROL_POINT_LENIENCY + 1) + yield return createSampleControlPointFor(nodeTime + LegacyBeatmapDecoder.CONTROL_POINT_LENIENCY + 1, hitObject.Samples); + } + } + else if (hitObject.Samples.Count > 0) + { + yield return createSampleControlPointFor(hitObject.GetEndTime(), hitObject.Samples); } foreach (var nested in collectSampleControlPoints(hitObject.NestedHitObjects)) yield return nested; } + + SampleControlPoint createSampleControlPointFor(double time, IList samples) + { + int volume = samples.Max(o => o.Volume); + int customIndex = samples.Any(o => o is ConvertHitObjectParser.LegacyHitSampleInfo) + ? samples.OfType().Max(o => o.CustomSampleBank) + : -1; + + return new LegacyBeatmapDecoder.LegacySampleControlPoint { Time = time, SampleVolume = volume, CustomSampleBank = customIndex }; + } } void extractSampleControlPoints(IEnumerable hitObject)