ppy / osu

rhythm is just a *click* away!
https://osu.ppy.sh
MIT License
15.45k stars 2.3k forks source link

Wrong BPM based SV rendering in mania #29661

Open TheEZIC opened 2 months ago

TheEZIC commented 2 months ago

Type

Game behaviour

Bug description

SV changes simulated using BPM changes are not displayed correctly

Screenshots or videos

Beatmap

https://osu.ppy.sh/beatmapsets/551469#mania/1168087

Beatmap timing point where this issue happens

00:40:915

stable

https://github.com/user-attachments/assets/6f0a9a85-12a3-42d6-9e37-fadc47194572

lazer

https://github.com/user-attachments/assets/09b5ecd7-3b56-46da-870e-97d091376278

Version

2024.817.0

Logs

compressed-logs.zip

bdach commented 1 month ago

This is happening because the map is (ab)using timing points in excess of 10,000 BPM to implement the "note freezing" effect. lazer parsing caps timing points to 10,000 BPM max which is nullifying the effects here.

Can be corroborated with

diff --git a/osu.Game/Beatmaps/ControlPoints/TimingControlPoint.cs b/osu.Game/Beatmaps/ControlPoints/TimingControlPoint.cs
index db1d440f18..096a0328e4 100644
--- a/osu.Game/Beatmaps/ControlPoints/TimingControlPoint.cs
+++ b/osu.Game/Beatmaps/ControlPoints/TimingControlPoint.cs
@@ -64,7 +64,7 @@ public bool OmitFirstBarLine
         /// </summary>
         public readonly BindableDouble BeatLengthBindable = new BindableDouble(DEFAULT_BEAT_LENGTH)
         {
-            MinValue = 6,
+            MinValue = 1,
             MaxValue = 60000
         };

I think this is a wontfix @ppy/team-client?

TheEZIC commented 1 month ago

I think this should be fixed because a lot of tournament and loved SV maps uses this 'feature'. If it's one line fix so i see no problem to fix that

peppy commented 1 month ago

I think this is a wontfix @ppy/team-client?

If making the proposed change does fix this without breaking things too bad, I'd probably just go with it for now.

bdach commented 1 month ago

To be clear: The change fixes this particular instance of the bug. If someone chooses to use a BPM of above 60,000, the bug will show again.

Do you still consider this one liner as acceptable in light of that?

peppy commented 1 month ago

@TheEZIC can you provide some examples of beatmaps where this is used? five or so would be appreciated.

TheEZIC commented 1 month ago

https://osu.ppy.sh/beatmapsets/1182702#mania/2465806 https://osu.ppy.sh/beatmapsets/501530#mania/1073952 https://osu.ppy.sh/beatmapsets/518951#mania/1102580 https://osu.ppy.sh/beatmapsets/697153#mania/1484721

Those maps use same trick but work well because they compensate BPM based SV with rate based SV: https://osu.ppy.sh/beatmapsets/376832#mania/824916 https://osu.ppy.sh/beatmapsets/728227#mania/1537498 https://osu.ppy.sh/beatmapsets/1838002#mania/3774040 https://osu.ppy.sh/beatmapsets/2253785#mania/4793998

Hope that would be enough. If necessary, I'll ask my friends who play tournaments for more

bdach commented 1 month ago

Just to illustrate my point above:

https://osu.ppy.sh/beatmapsets/1182702#mania/2465806 has timing points such as

83341,0.00864295644425315,4,1,0,20,1,0

https://osu.ppy.sh/beatmapsets/501530#mania/1073952 has

30333,0.6,4,2,0,0,1,0

https://osu.ppy.sh/beatmapsets/518951#mania/1102580 has

8250,0.1,4,1,0,20,1,0

https://osu.ppy.sh/beatmapsets/697153#mania/1484721 has

1246.9,0.01,4,1,0,0,1,0

All four would fail to "work properly" even with the patch attached above. For these to work reliably correctly, you'd have to pretty much stop enforcing any limits on beat length/BPM (except ensuring that it is positive, I guess). Which I guess could be done, but it's been here this long already and probably prevents some aspire tier bugs from surfacing that I'm not PRing removal of those until I am sure I have consensus on people being fine with it.