osu icon indicating copy to clipboard operation
osu copied to clipboard

Skip song intro on quick restart

Open BlauFx opened this issue 2 years ago • 11 comments

Closes #5745

~~Also to note: In case of a map restart I reduced the duration it takes for the PlayerLoader. I prefer having a shorter wait time on each map restart but that is just my personal preference so I'm not sure if this should be changed at all.~~

The song intro gets skipped if the user has requested a restart via the quick retry hotkey.

Before this PR:

https://user-images.githubusercontent.com/34138240/183213704-9d783a7d-7e8f-4d51-a70b-9a85890e76a5.mp4

~~After this PR (without the reduced wait time commit):~~

https://user-images.githubusercontent.com/34138240/183214156-b469b603-2f30-4d1a-b891-4e12e2aaf2c3.mp4

~~After this PR (including the reduced wait time commit):~~

https://user-images.githubusercontent.com/34138240/183214276-d8814df6-4991-47db-84f8-eb6557e2b0fe.mp4

After this PR:

https://user-images.githubusercontent.com/34138240/183254939-fdae75c1-7d3f-4fb8-8930-ce668120c9ea.mp4

BlauFx avatar Aug 05 '22 21:08 BlauFx

I don’t quite understand. This completely breaks multiple user interaction flows. How does a use change settings? How does a user restart without skipping if they want to have the intro time?

peppy avatar Aug 06 '22 02:08 peppy

I guess this should be like a specific key binding instead of game-wide function

Loreos7 avatar Aug 06 '22 07:08 Loreos7

I think this should only apply to quick restart, like it is in stable

uzervlad avatar Aug 06 '22 07:08 uzervlad

I don't know, but if this were to be implemented, there should be a toggle to enable it. The toggle could be under the gameplay tab in the options menu, somewhere near the fade playfield to red option.

nagi-desuuu avatar Aug 06 '22 09:08 nagi-desuuu

It also looks bad and I wouldn't see it getting merged in the current state (the player loading screen wasn't made to show for only 100ms).

peppy avatar Aug 06 '22 09:08 peppy

Retry feeling slow would be fixed when https://github.com/ppy/osu/issues/9039 gets implemented. This PR should only change skip behaviour

LittleEndu avatar Aug 06 '22 10:08 LittleEndu

Okay I did what @LittleEndu and @uzervlad suggested. This isn't breaking user interaction/UX. Are there any other issues?

BlauFx avatar Aug 06 '22 16:08 BlauFx

This PR requires test coverage. Please add tests.

peppy avatar Aug 08 '22 05:08 peppy

I don't think I agree with this behaviour in principle. Is it just me?

smoogipoo avatar Aug 09 '22 05:08 smoogipoo

The auto-skip as a whole? It's a stable behaviour which people expect to have available.

I'm fine with having it as a separate hotkey or similar, because I agree that not all users would want this. Open to ideas.

peppy avatar Aug 09 '22 05:08 peppy

Hmm, maybe I've played stable all these years and never noticed that was the case. If so I guess I can't complain.

smoogipoo avatar Aug 09 '22 05:08 smoogipoo

Could quick retrying just get rid of the whole player loading screen as a whole and just start at the start of the song just like stable? It doesn't really feel like a quick retry when it still takes a while to get back into the song.

AadilRn avatar Aug 15 '22 11:08 AadilRn

Yeah, that's something we've had in mind for a while.

smoogipoo avatar Aug 15 '22 11:08 smoogipoo

Had to rewrite every line (sorry).

@BlauFx please check my commits to see how I'd expect this to have been implemented. A couple of notes:

  • Your tests didn't actually test anything
  • You were using bindables in places where bindable flow was not required
  • As mentioned in my "i don't know what's going on" comment, the flow of transferring state between Player and PlayerLoder was weird. See how I've done it, hopefully it feels better.

Will require a fresh review from someone else

peppy avatar Aug 16 '22 05:08 peppy