Remove slider velocity from DrumRoll

[OliBomby]

Removed ISliderVelocity from DrumRoll because it doesnt need it and it created the ability to edit the SV in the editor which shouldn't be possible, because changing it only breaks the drum roll duration.

If you change the SV of a drum roll in the editor, save, and come back. The duration will have changed by a factor equal to how much you changed the SV from the scroll speed.

The SV was being used to calculate the scroll speed of the drum roll and it just happened to always be the same as the ScrollSpeed because of how LegacyBeatmapDecoder works. Removing SV and using scroll speed directly makes the intention of the code a lot clearer.

[smoogipoo]

Something tells me this is going to break elsewhere... For example this usage looks particularly suspect:

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

Lines 268 to 278 in d2baffa

	IEnumerable<DifficultyControlPoint> collectDifficultyControlPoints(IEnumerable<HitObject> hitObjects)
	{
	if (scrollSpeedEncodedAsSliderVelocity)
	yield break;
	foreach (var hitObject in hitObjects)
	{
	if (hitObject is IHasSliderVelocity hasSliderVelocity)
	yield return new DifficultyControlPoint { Time = hitObject.StartTime, SliderVelocity = hasSliderVelocity.SliderVelocity };
	}
	}

This is probably another manifestation of us over-complicating the beatmap format with control points, to a nigh point of unmaintainability, as I can't remember the reason why this was added in the first place. @peppy probably (hopefully) knows better than me.

[OliBomby]

Something tells me this is going to break elsewhere... For example this usage looks particularly suspect:

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

Lines 268 to 278 in d2baffa

	IEnumerable<DifficultyControlPoint> collectDifficultyControlPoints(IEnumerable<HitObject> hitObjects)
	{
	if (scrollSpeedEncodedAsSliderVelocity)
	yield break;
	foreach (var hitObject in hitObjects)
	{
	if (hitObject is IHasSliderVelocity hasSliderVelocity)
	yield return new DifficultyControlPoint { Time = hitObject.StartTime, SliderVelocity = hasSliderVelocity.SliderVelocity };
	}
	}

This is probably another manifestation of us over-complicating the beatmap format with control points, to a nigh point of unmaintainability, as I can't remember the reason why this was added in the first place. @peppy probably (hopefully) knows better than me.

The code you mentioned doesn't do anything in the flow of encoding taiko beatmaps because scrollSpeedEncodedAsSliderVelocity will be true. That means the slider velocity of hit objects doesn't get added to the legacy beatmap and instead the effect control point scroll speed gets added to the legacy beatmap, as slider velocity.

[bdach]

I dunno, this change looks correct to me. But at this point we've moved things back and forth so many times that I've lost track how we wanted to play this scroll speed vs slider velocity thing.

Probably gonna have to run like conversion or SR tests locally to confirm in practice.

[smoogipoo]

This change looks fine as far as my automated SR/PP tests observe: https://docs.google.com/spreadsheets/d/1bCKncunrtCtv5shXMPPb-jkpNZqXtKc7FQj7qrUQkgM/edit (sheet empty)

[bdach]

Relatively sure this is right

[peppy]

This is probably another manifestation of us over-complicating the beatmap format with control points, to a nigh point of unmaintainability, as I can't remember the reason why this was added in the first place. @peppy probably (hopefully) knows better than me.

To recap so we're on the same page:

• SliderVelocity is used for cases where an object's velocity is adjusted, and always local to the object
• ScrollSpeed is used for cases where the playfield's scroll speed is adjusted, causing external effects

As mentioned by @OliBomby in #24537 (comment), the code mentioned is not run for taiko legacy encoding. The intention of the code is to ferry the now local slider velocities in osu (and catch i guess?) to the correct place in the legacy format.

I don't think this is over-complicated. I think we're still moving in the correct direction to reduce complexity (moving away from global control points where possible, and maintaining the mapping only in decode/encode).

---

Also this change looks correct and was likely an oversight.