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

Fix occasional flash when quick exiting / retrying from player #30623

Merged
merged 5 commits into from
Nov 15, 2024

Conversation

peppy
Copy link
Member

@peppy peppy commented Nov 14, 2024

The gist of the issue is that fadeOut was being called twice in the quick exit/retry scenarios, causing weirdness with transforms.

I've restructured things to ensure it's only called once.

@peppy peppy requested a review from bdach November 14, 2024 12:48
@peppy peppy changed the title Fix occasional flash when query exiting / retrying from player Fix occasional flash when quick exiting / retrying from player Nov 14, 2024
@peppy peppy force-pushed the fix-flash-transition-player branch 3 times, most recently from b0e5d5e to 78a8d58 Compare November 14, 2024 12:54
The gist of the issue is that `fadeOut` was being called *twice* in the
quick exit/retry scenarios, causing weirdness with transforms.

I've restructured things to ensure it's only called once.
osu.Game/Screens/Play/Player.cs Outdated Show resolved Hide resolved
osu.Game/Screens/Play/Player.cs Outdated Show resolved Hide resolved
@bdach
Copy link
Collaborator

bdach commented Nov 14, 2024

How do I test that this does what it says? Do I need to?

@pull-request-size pull-request-size bot added size/M and removed size/S labels Nov 14, 2024
@peppy
Copy link
Member Author

peppy commented Nov 14, 2024

How do I test that this does what it says? Do I need to?

If you just want to confirm that this fixes the double-fadeOut call you can place a breakpoint in fadeOut on master and notice that it's always hit twice (once with instant=true and once with instant=false).

You could also try adjusting the values of the fadeout to repro the flash I was seeing, but maybe not worth it and just trust that I can no longer reproduce it. May be single thread only.

@bdach
Copy link
Collaborator

bdach commented Nov 14, 2024

Code seems fine now, that said, I'm gonna be a pain in the ass about this, and say that... this seems visually worse somehow?

master:

2024-11-14.15-21-16.mp4

this PR:

2024-11-14.15-19-39.mp4

Look at the playfield border specifically. It's doing this weird choppy fade now, rather than the smooth fade it is doing on master. I'm guessing that'll be caused by fadeOut() not getting called immediately in the two snipped call sites, and instead being shifted to On{Suspending,Exiting}().

@peppy
Copy link
Member Author

peppy commented Nov 14, 2024

That seems wrong. I'll re-check.

@peppy peppy requested a review from bdach November 14, 2024 16:07
Copy link
Collaborator

@bdach bdach left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems ok now

@bdach bdach merged commit 21d4076 into ppy:master Nov 15, 2024
8 of 10 checks passed
@peppy peppy deleted the fix-flash-transition-player branch November 20, 2024 03:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants