Slider tail volume is not saved properly

[TheMask3]

Type

Game behaviour

Bug description

Try changing slider volumes in editor, while their volume changes are indeed applied when you replay the beatmap in editor, their volumes are not actually saved. After you save the beatmap, quit, and enter editor again, all slider volumes are reset.

(I also noticed an editor performance regression where osu would easily stall for a few hundred milliseconds when there's a slider on timeline while replaying beatmap in editor, the worst spike I've seen lasts more than 3 seconds but that's another topic...)

Screenshots or videos

bandicam.2024-06-25.21-05-05-150.mp4

bandicam.2024-06-25.21-05-50-977.mp4

Version

2024.625.0-lazer

Logs

compressed-logs.zip

[bdach]

Just to confirm that we're looking at the same thing: I can reproduce this, but only when changing the volume of the slider tail.
The rest appears to work fine, and as far as I can see on your videos, the tail was the only thing that you were touching.

Is this correct? Is this the problem only with the volume of the tail, or are you seeing this for all slider node volumes and I need to keep looking?

[TheMask3]

Just to confirm that we're looking at the same thing: I can reproduce this, but only when changing the volume of the slider tail. The rest appears to work fine, and as far as I can see on your videos, the tail was the only thing that you were touching.

Is this correct? Is this the problem only with the volume of the tail, or are you seeing this for all slider node volumes and I need to keep looking?

Yep, middle node volumes are saved and further tests show their volumes are copied to tails after undo.

[bdach]

Okay, that's little solace, but at least it's not as bad as I initially feared.

Reason for this happening is that the tail samples are not actually attached to any hitobject:

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

Lines 280 to 284 in 6cd1367

	// The samples should be attached to the slider tail, however this can only be done if LastTick is removed otherwise they would play earlier than they're intended to.
	// (see mapping logic in `CreateNestedHitObjects` above)
	//
	// For now, the samples are played by the slider itself at the correct end time.
	TailSamples = this.GetNodeSamples(repeatCount + 1);

Therefore, the encoding logic that converts per-object samples to stable control points / "green lines" does not see tail samples at all:

osu/osu.Game/Beatmaps/Formats/LegacyBeatmapEncoder.cs

Lines 281 to 298 in 6cd1367

	IEnumerable<SampleControlPoint> collectSampleControlPoints(IEnumerable<HitObject> hitObjects)
	{
	foreach (var hitObject in hitObjects)
	{
	if (hitObject.Samples.Count > 0)
	{
	int volume = hitObject.Samples.Max(o => o.Volume);
	int customIndex = hitObject.Samples.Any(o => o is ConvertHitObjectParser.LegacyHitSampleInfo)
	? hitObject.Samples.OfType<ConvertHitObjectParser.LegacyHitSampleInfo>().Max(o => o.CustomSampleBank)
	: -1;
	yield return new LegacyBeatmapDecoder.LegacySampleControlPoint { Time = hitObject.GetEndTime(), SampleVolume = volume, CustomSampleBank = customIndex };
	}
	foreach (var nested in collectSampleControlPoints(hitObject.NestedHitObjects))
	yield return nested;
	}
	}

and thus ends up using the middle node volume.

The fix may be to just assign the samples to the tail hitobject after all, but ensure care that they don't play back at legacy last tick time as the relevant comment states...? @ppy/team-client @OliBomby thoughts, opinions?

[OliBomby]

The legacy encoder encodes sample information by putting control points on the end times of hitobjects and nested hitobjects. This is problematic for the the slider tail which coincides with the end time of the slider, because only one control point can exist at a time.

Also there is an asymmetry in the encoder/decoder that is partially responsible for this issue. The encoder recursively looks for samples in nested hitobjects while the decoder only looks at top-level hitobjects and checks if they inherit IHasRepeats (which is the more correct way to handle things imo since we cant trust nested hitobjects to have persistent state).

For a fix I think you have to solve this asymmetry and then implement a special offset for the last node sample so it doesnt overlap with the control point for the sliderbody samples.

[bdach]

The encoder recursively looks for samples in nested hitobjects while the decoder only looks at top-level hitobjects and checks if they inherit IHasRepeats (which is the more correct way to handle things imo since we cant trust nested hitobjects to have persistent state)

This is a fair shout actually. When investigating this I initially thought that that was weird and instinctively started writing the logic to encode NodeSamples for IHasRepeats instead, before catching myself and realising this normally is handled by nested objects.

The one slight problem with that is that you don't necessarily know when a node's time is. We can probably pretend it's not a problem and assume uniform duration distribution and be fine with using Duration / SpanCount()... until we're not (but that would probably only be a problem with something like a custom ruleset, I think for the main 4 rulesets it should be more than fine).

[OliBomby]

We can probably pretend it's not a problem and assume uniform duration distribution and be fine with using Duration / SpanCount()

We already made this assumption in many places like the repeats in the editor timeline or the legacy beatmap decoder. I think its safe even for custom rulesets, because the legacy format and hitobject interfaces lack the ability to encode non-uniform node samples.