Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong BPM based SV rendering in mania #29661

Open
TheEZIC opened this issue Aug 30, 2024 · 7 comments
Open

Wrong BPM based SV rendering in mania #29661

TheEZIC opened this issue Aug 30, 2024 · 7 comments

Comments

@TheEZIC
Copy link

TheEZIC commented Aug 30, 2024

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

stable.mp4

lazer

lazer.mp4

Version

2024.817.0

Logs

compressed-logs.zip

@bdach
Copy link
Collaborator

bdach commented Sep 25, 2024

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
Copy link
Author

TheEZIC commented Sep 25, 2024

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
Copy link
Member

peppy commented Sep 27, 2024

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
Copy link
Collaborator

bdach commented Sep 27, 2024

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
Copy link
Member

peppy commented Oct 2, 2024

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

@TheEZIC
Copy link
Author

TheEZIC commented Oct 2, 2024

@bdach
Copy link
Collaborator

bdach commented Oct 4, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants