ppy / osu

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

Autoplay visually jumps to next object on fast streams due to incorrect key up handling #22353

Open forteus19 opened 1 year ago

forteus19 commented 1 year ago

Type

Game behaviour

Bug description

On fast streams (300 BPM), hitcircles appear not to get hit while playing back in the editor. It also seems to happen only on certain maps, like Put an end and isogu. Other 300 BPM streams maps don't appear to be affected.

Screenshots or videos

https://user-images.githubusercontent.com/66690013/213919099-42ee1086-d7ea-4e45-92f5-2effff216e5e.mp4

Versions

2023.121.0-lazer 2023.123.0-lazer

Logs

runtime.log session.log

ekrctb commented 1 year ago

Clicks are skipped because FrameStablePlayback is always set to false for an editor even when it is not seeking the time.

https://github.com/ppy/osu/blob/d695214ae1ff955efc33d5b6be8fa9b193bb18b6/osu.Game/Rulesets/UI/FrameStabilityContainer.cs#L194-L197

I think frame stability should be ensured while playback / mouse wheel scrolling and only use the unstable playback while explicitly seeking. Notes are still missed after a seek, but it is much less prevalent. Ultimately, some other solutions are needed like recording judgement results in replays.

peppy commented 10 months ago

So, a few things:

Yes, this is fixed by frame stability, but only because frame stability allows replay frames to exist in a non-linear time scale. As in there are frames in the past, and frame stability will rewind to handle them correctly.

The base issue is that hitobjects are closer than 50 ms in these beatmaps, but auto generator adds a 50 ms minimum window for key-down-to-key-up:

https://github.com/ppy/osu/blob/e182acf3e8c012fe36f8317d70ca7d6abe1803e9/osu.Game.Rulesets.Osu/Replays/OsuAutoGenerator.cs#L293

I'd argue that this should be solved by fixing the auto generator to infer the next object and not hold for as long, or just reducing the key down period altogether to a value which will not cause issues like this.

Side note that DelayedMovements in autogenerator is currently never used and a dead code path...

Attempted solution here but why it doesn't work is explained in the commit message.