Remove `LegacyDifficultyControlPoint` / `LegacyBpmMultiplier` #24738

peppy · 2023-09-07T09:30:55Z

As pointed out in discussion, the flow of data surrounding these legacy pieces is ugly. It only got worse after difficulty data has been moved to hit objects themselves (with the worst code smell being LegacyBpmMultiplier existing in HitObject.

This PR removes all of this, instead locally computing the required precision-adjusted values where necessary. It also moves the still-required GenerateTicks to DifficultyControlPoint itself (we basically need to know if the BPM at point-in-time was NaN, but don't want to convey that in TimingControlPoint, so this is required to exist somewhere).

To make this work, I've also removed the precision limitations of SliderVelocityMultiplier. This doesn't seem to have an adverse effect on the editor luckily, so should play quite well.

This PR fixes a few issues which seemingly went silently unnoticed:

SliderVelocityMultiplier precision was not transferred to HitObject when the value is transferred
LegacyBpmMultiplier was not being transferred at all in OsuBeatmapConverter, so it was not being used anywhere (potentially affected difficulty calculation and gameplay)

I haven't added test coverage for these two cases because I'm not sure where to start for that.

bdach · 2023-09-07T17:00:48Z

Pushed a few menial fixups but looks good to me otherwise. That said, I haven't paid this much attention to the investigation, so definitely will want a @smoogipoo confirm on this one.

bdach · 2023-09-11T21:10:55Z

Tried to do sheets for this one.

The taiko changes are slightly worrying as I wouldn't expect anything to change there, but I have done zero investigation what that is so far. Could even be that I didn't set the generation up correctly on my machine, so reader beware.

pending check of the above

smoogipoo · 2023-09-12T02:07:42Z

Fwiw, taiko is where I'd expect most of the differences to surface, because the drumroll conversion code relies on super precise errors.

peppy · 2023-09-12T03:38:34Z

FWIW I'd expect there to be changes as I fixed several actual bugs. We'll need to confirm they are correct to our expectations as a result.

bdach · 2023-09-12T07:08:52Z

Well, there is at least one breakage on the mania side when compared with stable.

/b/2364269 with DT, 10K:

2364269.mp4

Appears to be caused by minuscule fluctuations of beatLength in the pattern generator:

10,16c10,16
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 26279: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 27779: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 29279: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 30029: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 31341: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 37904: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325684
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 38091: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325684
---
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 26279: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 27779: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 29279: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 30029: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 31341: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 37904: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325686
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 38091: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325686
74,79c74,79
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 71279: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325684
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 71654: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325684
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 72029: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325684
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 72404: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325684
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 72779: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325684
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 73904: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325684
---
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 71279: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325686
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 71654: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325686
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 72029: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325686
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 72404: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325686
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 72779: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325686
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 73904: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325686
99,104c99,104
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 86279: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 86841: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 87779: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 89279: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 89841: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 90779: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
---
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 86279: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 86841: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 87779: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 89279: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 89841: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 90779: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
114,120c114,120
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 26279: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 27779: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 29279: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 30029: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 31341: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 37904: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325684
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 38091: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325684
---
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 26279: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 27779: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 29279: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 30029: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 31341: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 37904: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325686
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 38091: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325686
178,183c178,183
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 71279: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325684
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 71654: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325684
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 72029: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325684
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 72404: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325684
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 72779: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325684
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 73904: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325684
---
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 71279: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325686
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 71654: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325686
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 72029: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325686
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 72404: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325686
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 72779: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325686
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 73904: DistanceObjectPatternGenerator calculated beatLength = 249.99999046325686
203,208c203,208
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 86279: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 86841: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 87779: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 89279: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 89841: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
< osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 90779: DistanceObjectPatternGenerator calculated beatLength = 499.9999809265137
\ No newline at end of file
---
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 86279: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 86841: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 87779: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 89279: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 89841: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
> osu.Game.Rulesets.Objects.Legacy.Osu.ConvertSlider @ 90779: DistanceObjectPatternGenerator calculated beatLength = 499.99998092651373
\ No newline at end of file

The underlying cause is probably that taking the reciprocal of LegacyBpmMultiplier as SliderVelocity is not lossless.

Haven't looked at the taiko cases yet, but I'm not sure doing so is worth the non-trivial effort anymore given that we know that this change technically breaks stuff now... (getting to the bottom of the above took me ~2hrs already).

Basically matching the old code more closely to avoid too much precision from doing math in a slightly different way.

peppy · 2023-09-12T09:43:34Z

I've adjusted things to the point it looks to match your output (to within ToString() imprecisions as discussed).

I think we can probably use this as a way to test github actions diffcalc runs, so hold off on the manual run until I give that a try (@smoogipoo has been working on updating the action and I'll re-deploy my local runner).

bdach · 2023-09-14T18:34:32Z

Map mentioned above looks to be back to the reference state now.

As discussed elsewhere, going to re-run the sheets overnight to verify.

bdach · 2023-09-15T08:11:20Z

New sheets:

Things look clean, so no further objections from me.

smoogipoo · 2023-09-15T08:17:19Z

osu.Game/Rulesets/Objects/Types/IHasSliderVelocity.cs

+            switch (rulesetShortName)
+            {
+                case "taiko":
+                case "mania":
+                    bpmMultiplier = sliderVelocityAsBeatLength < 0 ? Math.Clamp((float)-sliderVelocityAsBeatLength, 10, 10000) / 100.0 : 1;
+                    break;
+
+                default:
+                    bpmMultiplier = sliderVelocityAsBeatLength < 0 ? Math.Clamp((float)-sliderVelocityAsBeatLength, 10, 1000) / 100.0 : 1;
+                    break;
+            }


If this is intended as a method for all rulesets to use, do you think we should instead specify explicit osu and catch branches and leave the default branch as simple/lossless as possible?

Dunno. It may be sunk cost fallacy from all the overnight sheet running but I tend to think that this is okay. The fact that we're using legacy .osu format and encoding slider velocity into files in one of the most opaque ways I can imagine also contributes to this feeling - like if we were redoing this to be simpler, then we'd redefine the actual way the thing is stored to not require the weird division by 100.

Can wait until further work in my opinion.

I mean it from a different angle - this method is essentially a legacy method that pollutes an interface intended for any ruleset to use.

I can imagine any ruleset consumer would use this as a shortcut for timingPoint.BeatLength * SliderVelocityMultiplier without understanding the consequences of this - the precision and the weird clamping behaviour.

How do you explain the existence and purpose of this method to a ruleset consumer? I believe it should be more properly indicated because the xmldoc doesn't do it justice.

I see.

For one, it has to be public API, so that's that (unless we expose osu.Game internals to rulesets, but I'd rather not), but what'd work for you? More xmldoc? Moving to extension method? Marking as deprecated with inline disables?

I've moved it to an extension method. I've added more xmldoc, and I've added a throw on non-legacy use.

smoogipoo · 2023-09-15T08:51:09Z

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

+        /// Introduces floating-point errors to post-multiplied beat length for legacy rulesets that depend on it.
+        /// You should definitely not use this unless you know exactly what you're doing.
+        /// </summary>
+        public static double GetPrecisionAdjustedBeatLength(this IHasSliderVelocity hasSliderVelocity, TimingControlPoint timingControlPoint, string rulesetShortName)


I still don't like this being an extension method. We've gone from:

I don't see a reason why rulesets wouldn't use this method, and it'll cause precision loss
To:
I don't see a reason why rulesets wouldn't use this method, but it'll crash if they do

My suggestion was very explicit: have this return BeatLength * SliderVelocity for non-legacy rulesets. A second option I would be okay with is this being an explicit static class call LegacyRulesetExtensions.Get...().

I don't know. I don't think I'm being as unreasonable here as I'm being brought to believe. I defer to a 3rd opinion.

I would be okay with is this being an explicit static class call LegacyRulesetExtensions.Get...().

This sounds like a reasonable middle ground... @peppy?

Whatever works. I'm fine with any implementation as long as the method name remains intact, and this only works for legacy rulesets.

We discussed IRL and my rationale against making this work for every ruleset is that it would hide the floating point precision hacks in a seemingly benignly named method. Which is not great for readability and would need inline comments at each usage to make sense (which doesn't make much sense).

Here's the breaking change in the IScrollingInfo: ppy/osu#24550 And should remove the LegacyBpmMultiplier: ppy/osu#24738

peppy added 2 commits September 7, 2023 17:41

Remove all usage of LegacyDifficultyControlPoint

b34a36f

Remove precision limitations of SliderVelocity

dcbdc11

peppy added the area:beatmap-parsing .osu file format parsing label Sep 7, 2023

pull-request-size bot added the size/L label Sep 7, 2023

peppy mentioned this pull request Sep 7, 2023

Fix sliders not always being the correct length #24739

Merged

1 task

bdach added 2 commits September 7, 2023 18:47

Delete outdated warning disables

2bb8697

Remove braces

e71fef4

bdach previously approved these changes Sep 7, 2023

View reviewed changes

bdach requested a review from smoogipoo September 7, 2023 17:01

bdach assigned peppy Sep 7, 2023

Make rounding error even less precise

4ecc463

Basically matching the old code more closely to avoid too much precision from doing math in a slightly different way.

bdach previously approved these changes Sep 15, 2023

View reviewed changes

smoogipoo reviewed Sep 15, 2023

View reviewed changes

Move to extension method and throw on non-legacy ruleset

0031da7

peppy dismissed bdach’s stale review via 0031da7 September 15, 2023 08:39

smoogipoo reviewed Sep 15, 2023

View reviewed changes

Make not extension method

56cc2b6

bdach approved these changes Sep 15, 2023

View reviewed changes

Merge branch 'master' into no-legacy-difficulty-control-point

a6fab28

bdach enabled auto-merge September 15, 2023 09:14

bdach disabled auto-merge September 15, 2023 10:01

bdach merged commit 1ae8665 into ppy:master Sep 15, 2023

peppy deleted the no-legacy-difficulty-control-point branch September 15, 2023 11:00

andy840119 added a commit to andy840119/karaoke that referenced this pull request Sep 24, 2023

Upgrade the package to the latest and fix the breaking change.

94431e0

Here's the breaking change in the IScrollingInfo: ppy/osu#24550 And should remove the LegacyBpmMultiplier: ppy/osu#24738

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove `LegacyDifficultyControlPoint` / `LegacyBpmMultiplier` #24738

Remove `LegacyDifficultyControlPoint` / `LegacyBpmMultiplier` #24738

peppy commented Sep 7, 2023

bdach commented Sep 7, 2023

bdach commented Sep 11, 2023 •

edited

Loading

smoogipoo commented Sep 12, 2023 •

edited

Loading

peppy commented Sep 12, 2023

bdach commented Sep 12, 2023 •

edited by peppy

Loading

peppy commented Sep 12, 2023

bdach commented Sep 14, 2023

bdach commented Sep 15, 2023

smoogipoo Sep 15, 2023 •

edited

Loading

bdach Sep 15, 2023

smoogipoo Sep 15, 2023 •

edited

Loading

bdach Sep 15, 2023 •

edited

Loading

peppy Sep 15, 2023

smoogipoo Sep 15, 2023

bdach Sep 15, 2023

peppy Sep 15, 2023 •

edited

Loading

Remove LegacyDifficultyControlPoint / LegacyBpmMultiplier #24738

Remove LegacyDifficultyControlPoint / LegacyBpmMultiplier #24738

Conversation

peppy commented Sep 7, 2023

bdach commented Sep 7, 2023

bdach commented Sep 11, 2023 • edited Loading

smoogipoo commented Sep 12, 2023 • edited Loading

peppy commented Sep 12, 2023

bdach commented Sep 12, 2023 • edited by peppy Loading

peppy commented Sep 12, 2023

bdach commented Sep 14, 2023

bdach commented Sep 15, 2023

smoogipoo Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

bdach Sep 15, 2023

Choose a reason for hiding this comment

smoogipoo Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

bdach Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

peppy Sep 15, 2023

Choose a reason for hiding this comment

smoogipoo Sep 15, 2023

Choose a reason for hiding this comment

bdach Sep 15, 2023

Choose a reason for hiding this comment

peppy Sep 15, 2023 • edited Loading

Choose a reason for hiding this comment

Remove `LegacyDifficultyControlPoint` / `LegacyBpmMultiplier` #24738

Remove `LegacyDifficultyControlPoint` / `LegacyBpmMultiplier` #24738

bdach commented Sep 11, 2023 •

edited

Loading

smoogipoo commented Sep 12, 2023 •

edited

Loading

bdach commented Sep 12, 2023 •

edited by peppy

Loading

smoogipoo Sep 15, 2023 •

edited

Loading

smoogipoo Sep 15, 2023 •

edited

Loading

bdach Sep 15, 2023 •

edited

Loading

peppy Sep 15, 2023 •

edited

Loading